[RFC PATCH 3/6] usb: xhci-mtk: Do not use xhci's virt_dev in drop_endpoint
Chunfeng Yun (云春峰)
Chunfeng.Yun at mediatek.com
Tue Aug 24 02:15:46 PDT 2021
On Tue, 2021-08-24 at 11:53 +0800, Ikjoon Jang wrote:
> HI Chunfeng,
>
> On Mon, Aug 23, 2021 at 8:54 PM Chunfeng Yun <
> chunfeng.yun at mediatek.com> wrote:
> >
> > xhci-mtk depends on xhci's internal virt_dev when it retrieves its
> > internal data from usb_host_endpoint both in add_endpoint and
> > drop_endpoint callbacks. But when setup packet was retired by
> > transaction errors in xhci_setup_device() path, a virt_dev for the
> > slot
> > is newly created with real_port 0. This leads to xhci-mtks's NULL
> > pointer
> > dereference from drop_endpoint callback as xhci-mtk assumes that
> > virt_dev's
> > real_port is always started from one. The similar problems were
> > addressed
> > by [1] but that can't cover the failure cases from setup_device.
> >
> > This patch drops the usages of xhci's virt_dev in xhci-mtk's
> > drop_endpoint
> > callback by adopting hashtable for searching mtk's schedule entity
> > from a given usb_host_endpoint pointer instead of searching a
> > linked list.
> > So mtk's drop_endpoint callback doesn't have to rely on virt_dev at
> > all.
> >
> > [1] f351f4b63dac ("usb: xhci-mtk: fix oops when unbind driver")
> >
> > Signed-off-by: Ikjoon Jang <ikjn at chromium.org>
> > Signed-off-by: Chunfeng Yun <chunfeng.yun at mediatek.com>
> > ---
> > drivers/usb/host/xhci-mtk-sch.c | 100 ++++++++++++++++----------
> > ------
> > drivers/usb/host/xhci-mtk.h | 11 +++-
> > 2 files changed, 60 insertions(+), 51 deletions(-)
> >
> > diff --git a/drivers/usb/host/xhci-mtk-sch.c
> > b/drivers/usb/host/xhci-mtk-sch.c
> > index 54c521524bfc..f3c6f078f82d 100644
> > --- a/drivers/usb/host/xhci-mtk-sch.c
> > +++ b/drivers/usb/host/xhci-mtk-sch.c
> > @@ -80,7 +80,7 @@ decode_ep(struct usb_host_endpoint *ep, enum
> > usb_device_speed speed)
> > interval /= 1000;
> > }
> >
> > - snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d,
> > interval:%d/%d%s\n",
> > + snprintf(buf, DBG_BUF_EN, "%s ep%d%s %s, mpkt:%d,
> > interval:%d/%d%s",
> > usb_speed_string(speed), usb_endpoint_num(epd),
> > usb_endpoint_dir_in(epd) ? "in" : "out",
> > usb_ep_type_string(usb_endpoint_type(epd)),
> > @@ -129,6 +129,10 @@ get_bw_info(struct xhci_hcd_mtk *mtk, struct
> > usb_device *udev,
> > int bw_index;
> >
> > virt_dev = xhci->devs[udev->slot_id];
>
> Yeah, previous code had unnecessary NULL checking here.
yes, xhci_add/drop_endpoint() -> xhci_check_args() helps to check it,
it will return -EINVAL.
>
> > + if (!virt_dev->real_port) {
> > + WARN_ONCE(1, "%s invalid real_port\n",
> > dev_name(&udev->dev));
> > + return NULL;
> > + }
> >
> > if (udev->speed >= USB_SPEED_SUPER) {
> > if (usb_endpoint_dir_out(&ep->desc))
> > @@ -236,14 +240,20 @@ static void drop_tt(struct usb_device *udev)
> > }
> > }
> >
> > -static struct mu3h_sch_ep_info *create_sch_ep(struct usb_device
> > *udev,
> > - struct usb_host_endpoint *ep, struct xhci_ep_ctx *ep_ctx)
> > +static struct mu3h_sch_ep_info *
> > +create_sch_ep(struct xhci_hcd_mtk *mtk, struct usb_device *udev,
> > + struct usb_host_endpoint *ep, struct xhci_ep_ctx
> > *ep_ctx)
> > {
> > struct mu3h_sch_ep_info *sch_ep;
> > + struct mu3h_sch_bw_info *bw_info;
> > struct mu3h_sch_tt *tt = NULL;
> > u32 len_bw_budget_table;
> > size_t mem_size;
> >
> > + bw_info = get_bw_info(mtk, udev, ep);
> > + if (!bw_info)
> > + return ERR_PTR(-ENODEV);
> > +
> > if (is_fs_or_ls(udev->speed))
> > len_bw_budget_table = TT_MICROFRAMES_MAX;
> > else if ((udev->speed >= USB_SPEED_SUPER)
> > @@ -266,11 +276,13 @@ static struct mu3h_sch_ep_info
> > *create_sch_ep(struct usb_device *udev,
> > }
> > }
> >
> > + sch_ep->bw_info = bw_info;
> > sch_ep->sch_tt = tt;
> > sch_ep->ep = ep;
> > sch_ep->speed = udev->speed;
> > INIT_LIST_HEAD(&sch_ep->endpoint);
> > INIT_LIST_HEAD(&sch_ep->tt_endpoint);
> > + INIT_HLIST_NODE(&sch_ep->hentry);
>
> so tt_endpoint was revived, is this related to bugs?
No, it's no related with drop_endpoint issue, so keep it unchanged.
It helps to add debug info when encounter issue, we may want to find
all eps attached to a specific TT.
>
> >
> > return sch_ep;
> > }
> > @@ -576,9 +588,9 @@ static u32 get_esit_boundary(struct
> > mu3h_sch_ep_info *sch_ep)
> > return boundary;
> > }
> >
> > -static int check_sch_bw(struct mu3h_sch_bw_info *sch_bw,
> > - struct mu3h_sch_ep_info *sch_ep)
> > +static int check_sch_bw(struct mu3h_sch_ep_info *sch_ep)
> > {
> > + struct mu3h_sch_bw_info *sch_bw = sch_ep->bw_info;
> > const u32 esit_boundary = get_esit_boundary(sch_ep);
> > const u32 bw_boundary = get_bw_boundary(sch_ep->speed);
> > u32 offset;
> > @@ -624,23 +636,26 @@ static int check_sch_bw(struct
> > mu3h_sch_bw_info *sch_bw,
> > return load_ep_bw(sch_bw, sch_ep, true);
> > }
[skip]
> > diff --git a/drivers/usb/host/xhci-mtk.h b/drivers/usb/host/xhci-
> > mtk.h
> > index eb05aaf6edbe..dc5e83f5c5cc 100644
> > --- a/drivers/usb/host/xhci-mtk.h
> > +++ b/drivers/usb/host/xhci-mtk.h
> > @@ -10,11 +10,15 @@
> > #define _XHCI_MTK_H_
> >
> > #include <linux/clk.h>
> > +#include <linux/hashtable.h>
> >
> > #include "xhci.h"
> >
> > #define BULK_CLKS_NUM 5
> >
> > +/* support at most 64 ep, use 32 size hash table */
> > +#define SCH_EP_HASH_BITS 5
> > +
> > /**
> > * To simplify scheduler algorithm, set a upper limit for ESIT,
> > * if a synchromous ep's ESIT is larger than @XHCI_MTK_MAX_ESIT,
> > @@ -36,14 +40,12 @@ struct mu3h_sch_tt {
> > * struct mu3h_sch_bw_info: schedule information for bandwidth
> > domain
> > *
> > * @bus_bw: array to keep track of bandwidth already used at each
> > uframes
> > - * @bw_ep_list: eps in the bandwidth domain
> > *
> > * treat a HS root port as a bandwidth domain, but treat a SS root
> > port as
> > * two bandwidth domains, one for IN eps and another for OUT eps.
> > */
> > struct mu3h_sch_bw_info {
> > u32 bus_bw[XHCI_MTK_MAX_ESIT];
> > - struct list_head bw_ep_list;
> > };
> >
> > /**
> > @@ -54,8 +56,10 @@ struct mu3h_sch_bw_info {
> > * @num_budget_microframes: number of continuous uframes
> > * (@repeat==1) scheduled within the interval
> > * @bw_cost_per_microframe: bandwidth cost per microframe
> > + * @hentry: hash table entry
> > * @endpoint: linked into bandwidth domain which it belongs to
> > * @tt_endpoint: linked into mu3h_sch_tt's list which it belongs
> > to
> > + * @bw_info: bandwidth domain which this endpoint belongs
> > * @sch_tt: mu3h_sch_tt linked into
> > * @ep_type: endpoint type
> > * @maxpkt: max packet size of endpoint
> > @@ -84,7 +88,9 @@ struct mu3h_sch_ep_info {
> > u32 num_budget_microframes;
> > u32 bw_cost_per_microframe;
> > struct list_head endpoint;
> > + struct hlist_node hentry;
>
> I think a simple hashtable is well fit but also rhashtable
> had no problems to use.
Yes, rhashtable is also fine, but seems more complex, may introduce new
issues easier.
>
> Overall, I suggest you merge this patch with reverting
> b8731209958a "usb: xhci-mtk: Do not use xhci's virt_dev in
> drop_endpoint"
> with some notes about the bugs I made.
Ok, they are all related with error handling:
1. when usb_add_hcd() -> xhci_mtk_setup() -> xhci_mtk_ssusb_config() or
xhci_gen_setup() failed, xhci_mtk_sch_init() will not be called, but
probe() will call xhci_mtk_sch_exit(), then NULL dereference oops
happens, because rhashtable is not init but try to destroy it.
2. in xhci_mtk_sch_init(), when alloc sch_array failed, need destroy
rhashtable;
3. in add_ep_quirk(), when rhashtable_insert_fast() failed, need
destroy sch_ep(), it's not added in bw_ep_chk_list.
Thanks a lot
>
> IIUC, this patch looks like reviving mu3h_sch_tt::ep_list and
> changing rhash_table to hashtable. The incremental patch will be
> much shorter sized and clearer to understand.
> I'm not so clear what you are trying to fix.
>
> Besides that, I don't have any objections to these changes. :)
> Thanks.
>
> > struct list_head tt_endpoint;
> > + struct mu3h_sch_bw_info *bw_info;
> > struct mu3h_sch_tt *sch_tt;
> > u32 ep_type;
> > u32 maxpkt;
> > @@ -137,6 +143,7 @@ struct xhci_hcd_mtk {
> > struct usb_hcd *hcd;
> > struct mu3h_sch_bw_info *sch_array;
> > struct list_head bw_ep_chk_list;
> > + DECLARE_HASHTABLE(sch_ep_hash, SCH_EP_HASH_BITS);
> > struct mu3c_ippc_regs __iomem *ippc_regs;
> > int num_u2_ports;
> > int num_u3_ports;
> > --
> > 2.18.0
> >
More information about the Linux-mediatek
mailing list