linux-kernel.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Matthias Kaehlcke <mka@chromium.org>
To: Doug Anderson <dianders@chromium.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Alan Stern <stern@rowland.harvard.edu>,
	Rob Herring <robh+dt@kernel.org>,
	Frank Rowand <frowand.list@gmail.com>,
	Mathias Nyman <mathias.nyman@intel.com>,
	Felipe Balbi <balbi@kernel.org>,
	devicetree@vger.kernel.org, Peter Chen <peter.chen@kernel.org>,
	linux-kernel@vger.kernel.org, linux-usb@vger.kernel.org,
	Bastien Nocera <hadess@hadess.net>,
	Ravi Chandra Sadineni <ravisadineni@chromium.org>,
	Michal Simek <michal.simek@xilinx.com>,
	Roger Quadros <rogerq@kernel.org>,
	Krzysztof Kozlowski <krzk@kernel.org>,
	Stephen Boyd <swboyd@chromium.org>,
	Al Cooper <alcooperx@gmail.com>
Subject: Re: [PATCH v16 1/7] usb: misc: Add onboard_usb_hub driver
Date: Mon, 15 Nov 2021 18:08:32 -0800	[thread overview]
Message-ID: <YZMSoPg10xoZ5LYK@google.com> (raw)
In-Reply-To: <CAD=FV=U2OuZFrqzVfrbLOUM4nHXwr1uYAYZ9XYWMr-Q95gb_EA@mail.gmail.com>

Hi Doug,

thanks for the thorough review!

On Thu, Nov 11, 2021 at 03:31:31PM -0800, Doug Anderson wrote:
> Hi,
> 
> On Fri, Aug 13, 2021 at 12:52 PM Matthias Kaehlcke <mka@chromium.org> wrote:
> >
> > +++ b/Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub
> > @@ -0,0 +1,8 @@
> > +What:          /sys/bus/platform/devices/<dev>/always_powered_in_suspend
> > +Date:          March 2021
> > +KernelVersion: 5.13
> 
> I dunno how stuff like this is usually managed, but March 2021 and
> 5.13 is no longer correct.

will update, though it's not unlikely it will go stale again before this
series lands.

> > +ONBOARD USB HUB DRIVER
> > +M:     Matthias Kaehlcke <mka@chromium.org>
> > +L:     linux-usb@vger.kernel.org
> > +S:     Maintained
> > +F:     Documentation/devicetree/bindings/usb/onboard_usb_hub.yaml
> 
> I'm confused. Where is this .yaml file? It doesn't look landed and it
> doesn't look to be in your series.

It's a leftover from the early days of the series, when the driver had
it's own binding, I'll remove it.

> I guess this should be updated to:
> 
> F: Documentation/devicetree/bindings/usb/realtek,rts5411.yaml

Not sure about that, the rts5411 binding could exist without this driver.

> Also: should this have:
> 
> F: Documentation/ABI/testing/sysfs-bus-platform-onboard-usb-hub

ack

> > +struct udev_node {
> > +       struct usb_device *udev;
> > +       struct list_head list;
> > +};
> 
> nit: 'udev' has a whole different connotation to me. Maybe just go
> with `usbdev_node` ?

Will change to 'usbdev_dev' node as suggested, I think it's ok to keep
'udev' for the pointer to the USB device itself, since that abbreviation
is used commonly in USB kernel land.

> > +static int __maybe_unused onboard_hub_suspend(struct device *dev)
> > +{
> > +       struct onboard_hub *hub = dev_get_drvdata(dev);
> > +       struct udev_node *node;
> > +       bool power_off;
> > +       int rc = 0;
> > +
> > +       if (hub->always_powered_in_suspend)
> > +               return 0;
> > +
> > +       power_off = true;
> > +
> > +       mutex_lock(&hub->lock);
> > +
> > +       list_for_each_entry(node, &hub->udev_list, list) {
> > +               if (!device_may_wakeup(node->udev->bus->controller))
> > +                       continue;
> > +
> > +               if (usb_wakeup_enabled_descendants(node->udev)) {
> > +                       power_off = false;
> > +                       break;
> > +               }
> > +       }
> > +
> > +       mutex_unlock(&hub->lock);
> > +
> > +       if (power_off)
> > +               rc = onboard_hub_power_off(hub);
> > +
> > +       return rc;
> 
> optional nit: get rid of "rc" and write the above as:
> 
> if (power_off)
>   return onboard_hub_power_off(hub);
> 
> return 0;

ok, I plan to revert the suggested logic though and bail out 'early' if there
is nothing to do.

> > +static int __maybe_unused onboard_hub_resume(struct device *dev)
> > +{
> > +       struct onboard_hub *hub = dev_get_drvdata(dev);
> > +       int rc = 0;
> > +
> > +       if (!hub->is_powered_on)
> > +               rc = onboard_hub_power_on(hub);
> > +
> > +       return rc;
> 
> optional nit: get rid of "rc" and write the above as:
> 
> if (!hub->is_powered_on)
>   return onboard_hub_power_on(hub);
> 
> return 0;

ok, same as above

> > +static void onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev)
> > +{
> > +       struct udev_node *node;
> > +       char link_name[64];
> > +
> > +       snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev));
> > +       sysfs_remove_link(&hub->dev->kobj, link_name);
> 
> I would be at least moderately worried about the duplicate snprintf
> between here and the add function. Any way that could be a helper?

I'll add a helper

> > +static struct onboard_hub *_find_onboard_hub(struct device *dev)
> > +{
> > +       struct platform_device *pdev;
> > +       struct device_node *np;
> > +       phandle ph;
> > +
> > +       pdev = of_find_device_by_node(dev->of_node);
> > +       if (!pdev) {
> > +               if (of_property_read_u32(dev->of_node, "companion-hub", &ph)) {
> > +                       dev_err(dev, "failed to read 'companion-hub' property\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> > +
> > +               np = of_find_node_by_phandle(ph);
> > +               if (!np) {
> > +                       dev_err(dev, "failed to find device node for companion hub\n");
> > +                       return ERR_PTR(-EINVAL);
> > +               }
> 
> Aren't the above two calls equivalent to this?
> 
> npc = of_parse_phandle(dev->of_node, "companion-hub", 0)

Indeed, will use of_parse_phandle() instead

> > +
> > +               pdev = of_find_device_by_node(np);
> > +               of_node_put(np);
> > +
> > +               if (!pdev)
> > +                       return ERR_PTR(-EPROBE_DEFER);
> 
> Shouldn't you also defer if the dev_get_drvdata() returns NULL? What
> if you're racing the probe of the platform device?

Yeah, it seems that race could happen. IIUC we could use device_is_bound()
to check if probing completed, really_probe() calls driver_bound() only
after successfully probing the device.

> > +       }
> > +
> > +       put_device(&pdev->dev);
> > +
> > +       return dev_get_drvdata(&pdev->dev);
> 
> It feels like it would be safer to call dev_get_drvdata() before
> putting the device? ...and actually, are you sure you should even be
> putting the device? Maybe we should wait to put it until
> onboard_hub_usbdev_disconnect()

It shouldn't be necessary, when the platform device is destroyed it
unbinds the associated USB devices (see onboard_hub_remove()), hence
they don't keep using the drvdata. There was a related discussion in
the early days of this series: https://lkml.org/lkml/2020/9/21/2153

> > +static struct usb_device_driver onboard_hub_usbdev_driver = {
> > +
> > +       .name = "onboard-usb-hub",
> 
> Remove the extra blank line at the start of the structure?

ok

> > +void onboard_hub_create_pdevs(struct usb_device *parent_hub, struct list_head *pdev_list)
> > +{
> > +       int i;
> > +       phandle ph;
> > +       struct device_node *np, *npc;
> > +       struct platform_device *pdev;
> > +       struct pdev_list_entry *pdle;
> 
> Should the `INIT_LIST_HEAD(pdev_list);` go here? Is there any reason
> why we need to push this into the caller?

That would limit pdev_list to a single entry, which is not what we want. A
parent hub might have multiple compatible onboard hubs connected to it.

> > +       for (i = 1; i <= parent_hub->maxchild; i++) {
> > +               np = usb_of_get_device_node(parent_hub, i);
> > +               if (!np)
> > +                       continue;
> > +
> > +               if (!of_is_onboard_usb_hub(np))
> > +                       goto node_put;
> > +
> > +               if (of_property_read_u32(np, "companion-hub", &ph))
> > +                       goto node_put;
> > +
> > +               npc = of_find_node_by_phandle(ph);
> > +               if (!npc)
> > +                       goto node_put;
> 
> Aren't the above two calls equivalent to this?
> 
> npc = of_parse_phandle(np, "companion-hub", 0)

yes, will change to of_parse_phandle()

> I'm also curious why a companion-hub is a _required_ property.
> Couldn't you support USB 2.0 hubs better by just allowing
> companion-hub to be optional? I guess that could be a future
> improvement, but it also seems trivial to support from the start.

The evolution of this driver somewhat tied it to xHCI, however that
isn't strictly necessary. In a sense it is nice when 'companion-hub'
is mandatory, since things can get messy if it is forgotten when it
should be there.

The property should be mandatory in the bindings of the USB >= 3.0
hubs that are supported by this driver, but it could be optional
for USB 2.0 hubs. Instead of doing the enforcement in the driver
it could be limited to checking a DT against the bindings in .yaml.
It's also an option to make it mandatory in the driver through a
list of compatible strings / VIDs/PIDs.

> > +               pdev = of_find_device_by_node(npc);
> > +               of_node_put(npc);
> > +
> > +               if (pdev) {
> > +                       /* the companion hub already has a platform device, nothing to do here */
> > +                       put_device(&pdev->dev);
> > +                       goto node_put;
> > +               }
> > +
> > +               pdev = of_platform_device_create(np, NULL, &parent_hub->dev);
> > +               if (pdev) {
> > +                       pdle = kzalloc(sizeof(*pdle), GFP_KERNEL);
> 
> Maybe devm_kzalloc(&pdev->dev, GFP_KERNEL) ? Then you can get rid of
> the free in the destroy function?

it feels a bit sneaky to do it after creation instead of probe(), but I guess
it's fine.

> > +                       if (!pdle)
> > +                               goto node_put;
> 
> If your memory allocation fails here, don't you need to
> of_platform_device_destroy() ?

right, will call of_platform_device_destroy() in case of failure

> > +                       INIT_LIST_HEAD(&pdle->node);
> 
> I don't believe that the INIT_LIST_HEAD() does anything useful here.
> &pdle->node is not a list head--it's a list element. Adding it to the
> end of the existing list will fully initialize its ->next and ->prev
> pointers but won't look at what they were.

indeed, will remove

> > +                       pdle->pdev = pdev;
> > +                       list_add(&pdle->node, pdev_list);
> > +               } else {
> > +                       dev_err(&parent_hub->dev,
> > +                               "failed to create platform device for onboard hub '%s'\n",
> > +                               of_node_full_name(np));
> 
> Use "%pOF" instead of open-coding.

ack

> > +void onboard_hub_destroy_pdevs(struct list_head *pdev_list)
> > +{
> > +       struct pdev_list_entry *pdle, *tmp;
> > +
> > +       list_for_each_entry_safe(pdle, tmp, pdev_list, node) {
> > +               of_platform_device_destroy(&pdle->pdev->dev, NULL);
> > +               kfree(pdle);
> 
> It feels like you should be removing the node from the list too,
> right? Otherwise if you unbind / bind the USB driver you'll still have
> garbage in your list the 2nd time?

Could catch, it seems I limited testing to a single removal ...

  reply	other threads:[~2021-11-16  5:06 UTC|newest]

Thread overview: 28+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13 19:52 [PATCH v16 0/7] usb: misc: Add onboard_usb_hub driver Matthias Kaehlcke
2021-08-13 19:52 ` [PATCH v16 1/7] " Matthias Kaehlcke
2021-11-11 23:31   ` Doug Anderson
2021-11-16  2:08     ` Matthias Kaehlcke [this message]
2021-08-13 19:52 ` [PATCH v16 2/7] of/platform: Add stubs for of_platform_device_create/destroy() Matthias Kaehlcke
2021-08-13 19:52 ` [PATCH v16 3/7] ARM: configs: Explicitly enable USB_XHCI_PLATFORM where needed Matthias Kaehlcke
2021-08-26  6:45   ` Roger Quadros
2021-08-26  7:56   ` Krzysztof Kozlowski
2021-08-13 19:52 ` [PATCH v16 4/7] arm64: defconfig: Explicitly enable USB_XHCI_PLATFORM Matthias Kaehlcke
2021-08-26  6:46   ` Roger Quadros
2021-08-13 19:52 ` [PATCH v16 5/7] usb: Specify dependencies on USB_XHCI_PLATFORM with 'depends on' Matthias Kaehlcke
2021-08-26  6:46   ` Roger Quadros
2021-11-11 23:48   ` Doug Anderson
2021-11-16 18:07     ` Matthias Kaehlcke
2021-08-13 19:52 ` [PATCH v16 6/7] usb: host: xhci-plat: Create platform device for onboard hubs in probe() Matthias Kaehlcke
2021-10-20 13:05   ` Mathias Nyman
2021-10-20 20:27     ` Matthias Kaehlcke
2021-10-20 20:37       ` Alan Stern
2021-10-20 21:01         ` Matthias Kaehlcke
2021-10-20 21:57           ` Alan Stern
2021-08-13 19:52 ` [PATCH v16 7/7] arm64: dts: qcom: sc7180-trogdor: Add nodes for onboard USB hub Matthias Kaehlcke
2021-09-21 17:08 ` [PATCH v16 0/7] usb: misc: Add onboard_usb_hub driver Matthias Kaehlcke
2021-10-14 21:38   ` Doug Anderson
2021-10-15  6:39     ` Greg Kroah-Hartman
2021-10-19 16:04       ` Fabrice Gasnier
2021-10-19 22:10         ` Matthias Kaehlcke
2021-10-20  6:21         ` Michal Simek
2021-10-20 17:41           ` Matthias Kaehlcke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=YZMSoPg10xoZ5LYK@google.com \
    --to=mka@chromium.org \
    --cc=alcooperx@gmail.com \
    --cc=balbi@kernel.org \
    --cc=devicetree@vger.kernel.org \
    --cc=dianders@chromium.org \
    --cc=frowand.list@gmail.com \
    --cc=gregkh@linuxfoundation.org \
    --cc=hadess@hadess.net \
    --cc=krzk@kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-usb@vger.kernel.org \
    --cc=mathias.nyman@intel.com \
    --cc=michal.simek@xilinx.com \
    --cc=peter.chen@kernel.org \
    --cc=ravisadineni@chromium.org \
    --cc=robh+dt@kernel.org \
    --cc=rogerq@kernel.org \
    --cc=stern@rowland.harvard.edu \
    --cc=swboyd@chromium.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).