| Message ID | 87obmals06.fsf@tucsk.pomaz.szeredi.hu (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Headers | show |
On Thu, 16 Aug 2012, Miklos Szeredi wrote: > Yes, this appears to work. Following patch fixes the suspend oops. > > Thanks, > Miklos > > --- > drivers/ide/ide-pm.c | 4 ++-- > 1 files changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c > index 9240609..8d1e32d 100644 > --- a/drivers/ide/ide-pm.c > +++ b/drivers/ide/ide-pm.c > @@ -4,7 +4,7 @@ > > int generic_ide_suspend(struct device *dev, pm_message_t mesg) > { > - ide_drive_t *drive = dev_get_drvdata(dev); > + ide_drive_t *drive = to_ide_device(dev); > ide_drive_t *pair = ide_get_pair_dev(drive); > ide_hwif_t *hwif = drive->hwif; > struct request *rq; > @@ -40,7 +40,7 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg) > > int generic_ide_resume(struct device *dev) > { > - ide_drive_t *drive = dev_get_drvdata(dev); > + ide_drive_t *drive = to_ide_device(dev); > ide_drive_t *pair = ide_get_pair_dev(drive); > ide_hwif_t *hwif = drive->hwif; > struct request *rq; And now you can get rid of the useless dev_set_drvdata() call. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Thursday, August 16, 2012, Alan Stern wrote: > On Thu, 16 Aug 2012, Miklos Szeredi wrote: > > > Yes, this appears to work. Following patch fixes the suspend oops. > > > > Thanks, > > Miklos OK Miklos, can you please send that to Dave with a proper changelog and sign-off (please add my ACK too)? Please make it clear that this is a regression fix and which commit has introduced the regression. > > --- > > drivers/ide/ide-pm.c | 4 ++-- > > 1 files changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/ide/ide-pm.c b/drivers/ide/ide-pm.c > > index 9240609..8d1e32d 100644 > > --- a/drivers/ide/ide-pm.c > > +++ b/drivers/ide/ide-pm.c > > @@ -4,7 +4,7 @@ > > > > int generic_ide_suspend(struct device *dev, pm_message_t mesg) > > { > > - ide_drive_t *drive = dev_get_drvdata(dev); > > + ide_drive_t *drive = to_ide_device(dev); > > ide_drive_t *pair = ide_get_pair_dev(drive); > > ide_hwif_t *hwif = drive->hwif; > > struct request *rq; > > @@ -40,7 +40,7 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg) > > > > int generic_ide_resume(struct device *dev) > > { > > - ide_drive_t *drive = dev_get_drvdata(dev); > > + ide_drive_t *drive = to_ide_device(dev); > > ide_drive_t *pair = ide_get_pair_dev(drive); > > ide_hwif_t *hwif = drive->hwif; > > struct request *rq; > > And now you can get rid of the useless dev_set_drvdata() call. That was in the other patch I think. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi all, On 08/16/2012 10:02 PM, Rafael J. Wysocki wrote: > On Thursday, August 16, 2012, Alan Stern wrote: >> On Thu, 16 Aug 2012, Miklos Szeredi wrote: >> >>> Yes, this appears to work. Following patch fixes the suspend oops. >>> >>> Thanks, >>> Miklos > > OK > > Miklos, can you please send that to Dave with a proper changelog and > sign-off (please add my ACK too)? Please make it clear that this is a > regression fix and which commit has introduced the regression. I think that Alan's suggest fix, as implemented by Milos is great!, but before moving forward with this someone should audit all the other (generic) ide code for other places using the drvdata, a simple grep for dev_get_drvdata should show these. <snip> >> And now you can get rid of the useless dev_set_drvdata() call. > > That was in the other patch I think. No my patch was a hack to undo the results of the commit causing the regression in the IDE case. But Alan's approach clearly is much better! Once we are sure drvdata is not used anywhere the dev_set_drvdata call could be removed in the place where my hack added a second call to it. Note that there are likely actual ide drivers using it, without setting it themselves since the ide core was setting it. So removing it will require even more auditing / checking. A 3th thing which should be audited is the generic ide code assuming that dev->driver != NULL, this is at least true for generic_ide_remove, where: if (drv->remove) Should be changed to: if (dev->driver && drv->remove) Following the way things are done in generic_ide_shutdown, a similar change could be needed for generic_ide_probe(), although I guess that may never get called with dev->driver == NULL ? Unfortunately I'm very busy with other stuff atm, so I cannot help here other then pointing out that such audits should be done :| Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 17 Aug 2012, Hans de Goede wrote: > No my patch was a hack to undo the results of the commit causing > the regression in the IDE case. But Alan's approach clearly is > much better! Once we are sure drvdata is not used anywhere the > dev_set_drvdata call could be removed in the place where my > hack added a second call to it. Note that there are likely > actual ide drivers using it, without setting it themselves since > the ide core was setting it. So removing it will require even more > auditing / checking. I did search for dev_get_drvdata() calls in drivers/ide; there were no other calls that retrieved an ide_drive_t value. But I didn't check anywhere else in the kernel. In fact, I'm not sure where else to look. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 17 Aug 2012, Alan Stern wrote: > On Fri, 17 Aug 2012, Hans de Goede wrote: > > > No my patch was a hack to undo the results of the commit causing > > the regression in the IDE case. But Alan's approach clearly is > > much better! Once we are sure drvdata is not used anywhere the > > dev_set_drvdata call could be removed in the place where my > > hack added a second call to it. Note that there are likely > > actual ide drivers using it, without setting it themselves since > > the ide core was setting it. So removing it will require even more > > auditing / checking. > > I did search for dev_get_drvdata() calls in drivers/ide; there were no > other calls that retrieved an ide_drive_t value. But I didn't check > anywhere else in the kernel. In fact, I'm not sure where else to look. After a little more checking: Among all the .c files in the kernel which match the pattern '[^a-z]ide_', the only occurrences of dev_get_drvdata() which retrieve an ide_drive_t value are the two in ide-pm.c. Alan Stern -- To unsubscribe from this list: send the line "unsubscribe linux-pm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, On 08/17/2012 04:27 PM, Alan Stern wrote: > On Fri, 17 Aug 2012, Alan Stern wrote: > >> On Fri, 17 Aug 2012, Hans de Goede wrote: >> >>> No my patch was a hack to undo the results of the commit causing >>> the regression in the IDE case. But Alan's approach clearly is >>> much better! Once we are sure drvdata is not used anywhere the >>> dev_set_drvdata call could be removed in the place where my >>> hack added a second call to it. Note that there are likely >>> actual ide drivers using it, without setting it themselves since >>> the ide core was setting it. So removing it will require even more >>> auditing / checking. >> >> I did search for dev_get_drvdata() calls in drivers/ide; there were no >> other calls that retrieved an ide_drive_t value. But I didn't check >> anywhere else in the kernel. In fact, I'm not sure where else to look. > > After a little more checking: Among all the .c files in the kernel > which match the pattern '[^a-z]ide_', the only occurrences of > dev_get_drvdata() which retrieve an ide_drive_t value are the two in > ide-pm.c. Thanks, good work! That means we should move forward with submitting Miklos' patch to the ide subsystem maintainer asap. Also if someone feels like it we still could do the 2 follow up / clean up patches: 1) Removin the set_drvdata call from hwif_register_devices() in ide-prove.c 2) Fixup ide-generic.c ide_remove to not dereference a NULL dev->driver Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-pm" 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/ide/ide-pm.c b/drivers/ide/ide-pm.c index 9240609..8d1e32d 100644 --- a/drivers/ide/ide-pm.c +++ b/drivers/ide/ide-pm.c @@ -4,7 +4,7 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg) { - ide_drive_t *drive = dev_get_drvdata(dev); + ide_drive_t *drive = to_ide_device(dev); ide_drive_t *pair = ide_get_pair_dev(drive); ide_hwif_t *hwif = drive->hwif; struct request *rq; @@ -40,7 +40,7 @@ int generic_ide_suspend(struct device *dev, pm_message_t mesg) int generic_ide_resume(struct device *dev) { - ide_drive_t *drive = dev_get_drvdata(dev); + ide_drive_t *drive = to_ide_device(dev); ide_drive_t *pair = ide_get_pair_dev(drive); ide_hwif_t *hwif = drive->hwif; struct request *rq;