Messages in this thread | | | From | Saravana Kannan <> | Date | Thu, 26 Mar 2020 11:45:28 -0700 | Subject | Re: [PATCH v3] driver core: Break infinite loop when deferred probe can't be satisfied |
| |
On Thu, Mar 26, 2020 at 1:39 AM Rafael J. Wysocki <rafael@kernel.org> wrote: > > On Wed, Mar 25, 2020 at 11:09 PM Saravana Kannan <saravanak@google.com> wrote: > > > > On Wed, Mar 25, 2020 at 5:51 AM Andy Shevchenko > > <andriy.shevchenko@linux.intel.com> wrote: > > > > > [cut] > > > > > > > Yes, it's (unlikely) possible (*), but it will give one more iteration per such > > > case. It's definitely better than infinite loop. Do you agree? > > > > Sorry I wasn't being clear (I was in a rush). I'm saying this patch > > can reintroduce the bug where the deferred probe isn't triggered when > > it should be. > > > > Let's take a simple execution flow. > > > > probe_okay is at 10. > > > > Thread-A > > really_probe(Device-A) > > local_probe_okay_count = 10 > > Device-A probe function is running... > > > > Thread-B > > really_probe(Device-B) > > Device-B probes successfully. > > probe_okay incremented to 11 > > > > Thread-C > > Device-C (which had bound earlier) is unbound (say module is > > unloaded or a million other reasons). > > probe_okay is decremented to 10. > > > > Thread-A continues > > Device-A probe function returns -EPROBE_DEFER > > driver_deferred_probe_add_trigger() doesn't do anything because > > local_probe_okay_count == probe_okay > > But Device-A might have deferred probe waiting on Device-B. > > Device-A never probes. > > > > > *) It means during probe you have _intensive_ removing, of course you may keep > > > kernel busy with iterations, but it has no practical sense. DoS attacks more > > > effective in different ways. > > > > I wasn't worried about DoS attacks. More of a functional correctness > > issue what I explained above. > > The code is functionally incorrect as is already AFAICS. > > > Anyway, if your issue and similar issues can be handles in driver core > > in a clean way without breaking other cases, I don't have any problem > > with that. Just that, I think the current solution breaks other cases. > > OK, so the situation right now is that commit 58b116bce136 has > introduced a regression and so it needs to be fixed or reverted. The > cases that were previously broken and were unbroken by that commit > don't matter here, so you cannot argue that they would be "broken". > > It looks to me like the original issue fixed by the commit in question > needs to be addressed differently, so I would vote for reverting it > and starting over.
I'm fine with whatever approach. My only point is that code that's been there for 5+ years might be preventing that race in a multitude of platforms. So I'm just reviewing to make sure fixes aren't introducing regressions. I'm all for anyone cleaning up/redoing deferred probe.
> > As an alternate solution, assuming "linux,extcon-name" is coming > > from some firmware, you might want to look into the fw_devlink > > feature. > > That would be a workaround for a driver core issue, though, wouldn't it?
I'm not saying don't fix it in the driver core if it can be done without adding regressions.
> > That feature allows driver core to add device links from firmware > > information. If you can get that feature to create device links from > > your dwc3.0.auto (or its parent pci_dev?) to the extcon supplier > > device, all of this can be sidestepped and your dwc3.0.auto's (or the > > dwc pci_dev's) probe will be triggered only after extcon is probed. > > > > I have very little familiarity with PCI/ACPI. I spent about an hour or > > two poking at ACPI scan/property code. The relationship between a > > pci_dev and an acpi_device is a bit confusing to me because I see: > > > > static int dwc3_pci_probe(struct pci_dev *pci, const struct pci_device_id *id) > > { > > struct property_entry *p = (struct property_entry *)id->driver_data; > > struct dwc3_pci *dwc; > > struct resource res[2]; > > int ret; > > struct device *dev = &pci->dev; > > .... > > dwc->dwc3 = platform_device_alloc("dwc3", PLATFORM_DEVID_AUTO); > > .... > > ACPI_COMPANION_SET(&dwc->dwc3->dev, ACPI_COMPANION(dev)); > > > > And ACPI_COMPANION returns an acpi_device by looking at dev->fwnode. > > So how the heck is a pci_device.dev.fwnode pointing to an > > acpi_device.fwnode? > > acpi_device is an of_node counterpart (or it is an fwnode itself if you will).
If I understand correctly, you are saying it's similar to struct device_node for OF -- as in, a data struct that stores the unpacked ACPI firmware data. That helps me understand what is going on with ACPI_COMPANION_SET() in the PCI driver.
But then, why does it have a "struct device dev" field embedded in it? Does the acpi_device.dev ever get registered with driver core?
Thanks, Saravana
| |