Message ID | 51C26861.9050106@intel.com |
---|---|
State | Not Applicable |
Delegated to: | David Miller |
Headers | show |
Hello. On 20-06-2013 6:26, Aaron Lu wrote: >>>>> From: Liu Jiang <liu97@gmail.com> >>>>> >>>>> Commit 30dcf76acc69 "libata: migrate ACPI code over to new bindings" >>>>> removed ACPI dock notification related code, but there's some dead >>>>> code left, so clean up it. [...] > Subject: [PATCH] libata-acpi: add back ACPI based hotplug functionality > Commit 30dcf76acc mistakenly dropped the code to register hotplug Please specify the summary line for this commit, the same as was done by Liu above (may use parens instead of quotes). > notificaion handler for ATA port/devices, causing regression for people > using ATA bay, as bug #59871 shows. > Fix this by adding back the hotplug notification handler registration > code. Since this code has to be run once and notification needs to be > installed on every ATA port/devices handle no matter if there is actual > device attached, we can't do this in binding time for ATA device ACPI > handle, as the binding only occurs when a SCSI device is created, i.e. > there is device attached. So introduced the ata_acpi_hotplug_init > function to loop scan all ATA ACPI handles and if it is available, > install the notification handler for it during ATA init time. > With the ATA ACPI handle binding to SCSI device tree, it is possible now > that when the SCSI hotplug work removes the SCSI device, the ACPI unbind > function will find that the corresponding ACPI device has already been > deleted by dock driver, causing a scaring message like: > [ 128.263966] scsi 4:0:0:0: Oops, 'acpi_handle' corrupt > Fix this by waiting for SCSI hotplug task finish in our notificaion > handler, so that the removal of ACPI device done in ACPI unbind function > triggered by the removal of SCSI device is run earlier when ACPI device > is still available. > References: https://bugzilla.kernel.org/show_bug.cgi?id=59871 > Reported-and-bisected-by: Dirk Griesbach <spamthis@freenet.de> > Cc: <stable@vger.kernel.org> > Signed-off-by: Aaron Lu <aaron.lu@intel.com> > --- > drivers/ata/libata-acpi.c | 34 +++++++++++++++++++++++++++++++++- > drivers/ata/libata-core.c | 2 ++ > drivers/ata/libata.h | 2 ++ > 3 files changed, 37 insertions(+), 1 deletion(-) > diff --git a/drivers/ata/libata-acpi.c b/drivers/ata/libata-acpi.c > index 87f2f39..0ad54bb 100644 > --- a/drivers/ata/libata-acpi.c > +++ b/drivers/ata/libata-acpi.c [...] > @@ -214,6 +216,36 @@ static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { > .uevent = ata_acpi_ap_uevent, > }; > > +void ata_acpi_hotplug_init(struct ata_host *host) > +{ > + int i; > + > + for (i = 0; i < host->n_ports; i++) { > + struct ata_port *ap = host->ports[i]; > + acpi_handle handle; > + struct ata_device *dev; > + > + if (!ap) > + continue; > + > + handle = ata_ap_acpi_handle(ap); > + if (handle) { > + /* we might be on a docking station */ > + register_hotplug_dock_device(handle, > + &ata_acpi_ap_dock_ops, ap); Please indent this line under the next character after ( above. > + } > + > + ata_for_each_dev(dev, &ap->link, ALL) { > + handle = ata_dev_acpi_handle(dev); > + if (handle) { > + /* we might be on a docking station */ > + register_hotplug_dock_device(handle, > + &ata_acpi_dev_dock_ops, dev); Same comment. WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/20/2013 07:02 PM, Sergei Shtylyov wrote: > Hello. Hi, Thanks for your comments. > > On 20-06-2013 6:26, Aaron Lu wrote: >> >> +void ata_acpi_hotplug_init(struct ata_host *host) >> +{ >> + int i; >> + >> + for (i = 0; i < host->n_ports; i++) { >> + struct ata_port *ap = host->ports[i]; >> + acpi_handle handle; >> + struct ata_device *dev; >> + >> + if (!ap) >> + continue; >> + >> + handle = ata_ap_acpi_handle(ap); >> + if (handle) { >> + /* we might be on a docking station */ >> + register_hotplug_dock_device(handle, >> + &ata_acpi_ap_dock_ops, ap); > > Please indent this line under the next character after ( above. Is there a link about this rule? I might have missed something about coding style. Thanks, Aaron > >> + } >> + >> + ata_for_each_dev(dev, &ap->link, ALL) { >> + handle = ata_dev_acpi_handle(dev); >> + if (handle) { >> + /* we might be on a docking station */ >> + register_hotplug_dock_device(handle, >> + &ata_acpi_dev_dock_ops, dev); > > Same comment. > > WBR, Sergei > -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, On Fri, Jun 21, 2013 at 08:55:37AM +0800, Aaron Lu wrote: > > Please indent this line under the next character after ( above. > > Is there a link about this rule? I might have missed something about > coding style. I don't think there's an explicit rule about that and I don't follow it religiously but I think it does make things easier on the eyes and emacs does that by default so I just got used to it. It's a very minor thing but FWIW I prefer things that way. Thanks.
On 06/21/2013 02:29 PM, Tejun Heo wrote: > Hello, > > On Fri, Jun 21, 2013 at 08:55:37AM +0800, Aaron Lu wrote: >>> Please indent this line under the next character after ( above. >> >> Is there a link about this rule? I might have missed something about >> coding style. > > I don't think there's an explicit rule about that and I don't follow > it religiously but I think it does make things easier on the eyes and > emacs does that by default so I just got used to it. It's a very > minor thing but FWIW I prefer things that way. OK, will refresh the patch and send out again. Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello. On 21-06-2013 4:55, Aaron Lu wrote: >>> +void ata_acpi_hotplug_init(struct ata_host *host) >>> +{ >>> + int i; >>> + >>> + for (i = 0; i < host->n_ports; i++) { >>> + struct ata_port *ap = host->ports[i]; >>> + acpi_handle handle; >>> + struct ata_device *dev; >>> + >>> + if (!ap) >>> + continue; >>> + >>> + handle = ata_ap_acpi_handle(ap); >>> + if (handle) { >>> + /* we might be on a docking station */ >>> + register_hotplug_dock_device(handle, >>> + &ata_acpi_ap_dock_ops, ap); >> Please indent this line under the next character after ( above. > Is there a link about this rule? I might have missed something about > coding style. Don't think so. This is a rule in some subsystems like networking, and it's also the way Emacs does such things. So, in principle, you can ignore my comment (although libata seems to also use this style). WBR, Sergei -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Fri, 2013-06-21 at 08:55 +0800, Aaron Lu wrote: > On 06/20/2013 07:02 PM, Sergei Shtylyov wrote: > > Hello. > > Hi, > > Thanks for your comments. > > > > > On 20-06-2013 6:26, Aaron Lu wrote: > >> > >> +void ata_acpi_hotplug_init(struct ata_host *host) > >> +{ > >> + int i; > >> + > >> + for (i = 0; i < host->n_ports; i++) { > >> + struct ata_port *ap = host->ports[i]; > >> + acpi_handle handle; > >> + struct ata_device *dev; > >> + > >> + if (!ap) > >> + continue; > >> + > >> + handle = ata_ap_acpi_handle(ap); > >> + if (handle) { > >> + /* we might be on a docking station */ > >> + register_hotplug_dock_device(handle, > >> + &ata_acpi_ap_dock_ops, ap); > > > > Please indent this line under the next character after ( above. > > Is there a link about this rule? I might have missed something about > coding style. The rule is follow the coding style in the file, unless there's something really wrong with it (which there might be in the case of really old drivers). The reason is that a mixture of coding styles makes the file much harder to read than a single consistent style. In this case, if you look at libata-acpi.c you see all continuation lines of function calls are aligned with open braces. James -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2013 11:25 PM, James Bottomley wrote: > On Fri, 2013-06-21 at 08:55 +0800, Aaron Lu wrote: >> On 06/20/2013 07:02 PM, Sergei Shtylyov wrote: >>> Hello. >> >> Hi, >> >> Thanks for your comments. >> >>> >>> On 20-06-2013 6:26, Aaron Lu wrote: >>>> >>>> +void ata_acpi_hotplug_init(struct ata_host *host) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < host->n_ports; i++) { >>>> + struct ata_port *ap = host->ports[i]; >>>> + acpi_handle handle; >>>> + struct ata_device *dev; >>>> + >>>> + if (!ap) >>>> + continue; >>>> + >>>> + handle = ata_ap_acpi_handle(ap); >>>> + if (handle) { >>>> + /* we might be on a docking station */ >>>> + register_hotplug_dock_device(handle, >>>> + &ata_acpi_ap_dock_ops, ap); >>> >>> Please indent this line under the next character after ( above. >> >> Is there a link about this rule? I might have missed something about >> coding style. > > The rule is follow the coding style in the file, unless there's > something really wrong with it (which there might be in the case of > really old drivers). The reason is that a mixture of coding styles > makes the file much harder to read than a single consistent style. Oh right, that's the rule I missed. Thanks for letting me know. > > In this case, if you look at libata-acpi.c you see all continuation > lines of function calls are aligned with open braces. Indeed. -Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 06/21/2013 07:35 PM, Sergei Shtylyov wrote: > Hello. > > On 21-06-2013 4:55, Aaron Lu wrote: > >>>> +void ata_acpi_hotplug_init(struct ata_host *host) >>>> +{ >>>> + int i; >>>> + >>>> + for (i = 0; i < host->n_ports; i++) { >>>> + struct ata_port *ap = host->ports[i]; >>>> + acpi_handle handle; >>>> + struct ata_device *dev; >>>> + >>>> + if (!ap) >>>> + continue; >>>> + >>>> + handle = ata_ap_acpi_handle(ap); >>>> + if (handle) { >>>> + /* we might be on a docking station */ >>>> + register_hotplug_dock_device(handle, >>>> + &ata_acpi_ap_dock_ops, ap); > >>> Please indent this line under the next character after ( above. > >> Is there a link about this rule? I might have missed something about >> coding style. > > Don't think so. This is a rule in some subsystems like networking, > and it's also the way Emacs does such things. So, in principle, you can > ignore my comment (although libata seems to also use this style). FYI, the updated patch has been sent by Rafael for me: http://marc.info/?l=linux-acpi&m=137207724507019&w=2 with your suggestion taken. Thanks, Aaron -- To unsubscribe from this list: send the line "unsubscribe linux-ide" 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/ata/libata-acpi.c b/drivers/ata/libata-acpi.c index 87f2f39..0ad54bb 100644 --- a/drivers/ata/libata-acpi.c +++ b/drivers/ata/libata-acpi.c @@ -156,8 +156,10 @@ static void ata_acpi_handle_hotplug(struct ata_port *ap, struct ata_device *dev, spin_unlock_irqrestore(ap->lock, flags); - if (wait) + if (wait) { ata_port_wait_eh(ap); + flush_work(&ap->hotplug_task.work); + } } static void ata_acpi_dev_notify_dock(acpi_handle handle, u32 event, void *data) @@ -214,6 +216,36 @@ static const struct acpi_dock_ops ata_acpi_ap_dock_ops = { .uevent = ata_acpi_ap_uevent, }; +void ata_acpi_hotplug_init(struct ata_host *host) +{ + int i; + + for (i = 0; i < host->n_ports; i++) { + struct ata_port *ap = host->ports[i]; + acpi_handle handle; + struct ata_device *dev; + + if (!ap) + continue; + + handle = ata_ap_acpi_handle(ap); + if (handle) { + /* we might be on a docking station */ + register_hotplug_dock_device(handle, + &ata_acpi_ap_dock_ops, ap); + } + + ata_for_each_dev(dev, &ap->link, ALL) { + handle = ata_dev_acpi_handle(dev); + if (handle) { + /* we might be on a docking station */ + register_hotplug_dock_device(handle, + &ata_acpi_dev_dock_ops, dev); + } + } + } +} + /** * ata_acpi_dissociate - dissociate ATA host from ACPI objects * @host: target ATA host diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index f218427..adf002a 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -6148,6 +6148,8 @@ int ata_host_register(struct ata_host *host, struct scsi_host_template *sht) if (rc) goto err_tadd; + ata_acpi_hotplug_init(host); + /* set cable, sata_spd_limit and report */ for (i = 0; i < host->n_ports; i++) { struct ata_port *ap = host->ports[i]; diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index c949dd3..577d902b 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -122,6 +122,7 @@ extern int ata_acpi_register(void); extern void ata_acpi_unregister(void); extern void ata_acpi_bind(struct ata_device *dev); extern void ata_acpi_unbind(struct ata_device *dev); +extern void ata_acpi_hotplug_init(struct ata_host *host); #else static inline void ata_acpi_dissociate(struct ata_host *host) { } static inline int ata_acpi_on_suspend(struct ata_port *ap) { return 0; } @@ -134,6 +135,7 @@ static inline int ata_acpi_register(void) { return 0; } static inline void ata_acpi_unregister(void) { } static inline void ata_acpi_bind(struct ata_device *dev) { } static inline void ata_acpi_unbind(struct ata_device *dev) { } +static inline void ata_acpi_hotplug_init(struct ata_host *host) {} #endif /* libata-scsi.c */