| Message ID | 20170225172357.26294-3-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
HI, On 25-02-17 18:23, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. > > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. This paragraph of the commit msg should be removed, I dropped that part of the patch since acpi_init_device_object() already sets status and all acpi_device's get that called upon creation. Regards, Hans > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 25/02/17 19:23, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. > > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Acked-by: Adrian Hunter <adrian.hunter@intel.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Adrian, On Sat, 25 Feb 2017 18:23:56 +0100, Hans de Goede wrote: > Calling acpi_device_fix_up_power() on a device which is not present > is not a good idea. How bad is it? This was introduced by commit e5bbf30733f9, which was backported to several stable branches. If it causes real trouble then this fix-up patch should be annotated with Fixes: e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are powered when") and Cc's to stable@, so it can be propagated to all affected trees. > While at it also call acpi_bus_get_status() on the children before > the status check to make sure that child->status contains valid data. > > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/mmc/host/sdhci-acpi.c | 6 +++--- > 1 file changed, 3 insertions(+), 3 deletions(-) > > diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c > index 96465ff..873beae 100644 > --- a/drivers/mmc/host/sdhci-acpi.c > +++ b/drivers/mmc/host/sdhci-acpi.c > @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) > if (acpi_bus_get_device(handle, &device)) > return -ENODEV; > > + if (acpi_bus_get_status(device) || !device->status.present) > + return -ENODEV; > + > /* Power on the SDHCI controller and its children */ > acpi_device_fix_up_power(device); > list_for_each_entry(child, &device->children, node) > if (child->status.present && child->status.enabled) > acpi_device_fix_up_power(child); > > - if (acpi_bus_get_status(device) || !device->status.present) > - return -ENODEV; > - > if (sdhci_acpi_byt_defer(dev)) > return -EPROBE_DEFER; >
Hi, On 03-03-17 10:09, Jean Delvare wrote: > Hi Hans, Adrian, > > On Sat, 25 Feb 2017 18:23:56 +0100, Hans de Goede wrote: >> Calling acpi_device_fix_up_power() on a device which is not present >> is not a good idea. > > How bad is it? Not that bad really, thinking more about this, since drivers/mmc/host/sdhci-acpi.c is a platform driver, its probe function will never get called if !device->status.present, since the acpi-subsys then will not instantiate a platform device for the acpi-dev in question at all. So a better fix would be to just drop the superfluous check. I'll send a new version doing this. Regards, Hans > > This was introduced by commit e5bbf30733f9, which was backported to > several stable branches. If it causes real trouble then this fix-up > patch should be annotated with > > Fixes: e5bbf30733f9 ("mmc: sdhci-acpi: Ensure connected devices are powered when") > > and Cc's to stable@, so it can be propagated to all affected trees. > >> While at it also call acpi_bus_get_status() on the children before >> the status check to make sure that child->status contains valid data. >> >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/mmc/host/sdhci-acpi.c | 6 +++--- >> 1 file changed, 3 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c >> index 96465ff..873beae 100644 >> --- a/drivers/mmc/host/sdhci-acpi.c >> +++ b/drivers/mmc/host/sdhci-acpi.c >> @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) >> if (acpi_bus_get_device(handle, &device)) >> return -ENODEV; >> >> + if (acpi_bus_get_status(device) || !device->status.present) >> + return -ENODEV; >> + >> /* Power on the SDHCI controller and its children */ >> acpi_device_fix_up_power(device); >> list_for_each_entry(child, &device->children, node) >> if (child->status.present && child->status.enabled) >> acpi_device_fix_up_power(child); >> >> - if (acpi_bus_get_status(device) || !device->status.present) >> - return -ENODEV; >> - >> if (sdhci_acpi_byt_defer(dev)) >> return -EPROBE_DEFER; >> > > -- To unsubscribe from this list: send the line "unsubscribe linux-mmc" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/mmc/host/sdhci-acpi.c b/drivers/mmc/host/sdhci-acpi.c index 96465ff..873beae 100644 --- a/drivers/mmc/host/sdhci-acpi.c +++ b/drivers/mmc/host/sdhci-acpi.c @@ -394,15 +394,15 @@ static int sdhci_acpi_probe(struct platform_device *pdev) if (acpi_bus_get_device(handle, &device)) return -ENODEV; + if (acpi_bus_get_status(device) || !device->status.present) + return -ENODEV; + /* Power on the SDHCI controller and its children */ acpi_device_fix_up_power(device); list_for_each_entry(child, &device->children, node) if (child->status.present && child->status.enabled) acpi_device_fix_up_power(child); - if (acpi_bus_get_status(device) || !device->status.present) - return -ENODEV; - if (sdhci_acpi_byt_defer(dev)) return -EPROBE_DEFER;
Calling acpi_device_fix_up_power() on a device which is not present is not a good idea. While at it also call acpi_bus_get_status() on the children before the status check to make sure that child->status contains valid data. Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/mmc/host/sdhci-acpi.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)