Re: [PATCH] ahci: unregister acpi notify handler when a ZPODD is unbound

[Bartlomiej Zolnierkiewicz <b.zolnierkie@samsung.com>]

Hi,

On Wednesday, April 30, 2014 06:04:47 PM Levente Kurusa wrote:
> When a ZPODD device is unbound via sysfs, the acpi notify handler
> is not removed. This causes panics as observed in Bug #74601. The
> panic only happens when the wake happens from outside the kernel
> (i.e. inserting media or pressing a button). Implement a new
> ahci_remove_one function which causes zpodd_exit to be called for all
> ZPODD devices on the unbound PCI device.
> 
> Signed-off-by: Levente Kurusa <levex@linux.com>
> ---
> 
> Hi,
> 
> I am not sure if the loop below is correct. Maybe there is a better
> solution to loop through all the devices which might use ZPODD?
> 
> Thanks, Lev.
> 
>  drivers/ata/ahci.c         | 21 +++++++++++++++++++++
>  drivers/ata/ahci.h         |  4 ++++
>  drivers/ata/libata-zpodd.c |  1 +
>  3 files changed, 26 insertions(+)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index 5a0bf8e..6d92bc9 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -475,12 +475,33 @@ static const struct pci_device_id ahci_pci_tbl[] = {
>  	{ }	/* terminate list */
>  };
>  
> +#ifdef CONFIG_SATA_ZPODD
> +void ahci_remove_one(struct pci_dev *pdev)
> +{
> +	struct ata_host *host = pci_get_drvdata(pdev);
> +	struct ata_link *link;
> +	struct ata_device *dev;
> +	int i;
> +
> +	for (i = 0; i < host->n_ports; i++)
> +		ata_for_each_link(link, host->ports[i], HOST_FIRST)
> +			ata_for_each_dev(dev, link, ALL)
> +				if (dev->zpodd)
> +					zpodd_exit(dev);

zpodd_init() is used in generic libata library code, same should be true
for zpodd_exit().  Ideally, you should find some libata library function
called from ata_pci_remove_one() suitable for this purpose.

Hmm, actually zpodd_exit() is already used in ata_scsi_handle_link_detach()
so this should also be taken into consideration..

> +	ata_pci_remove_one(pdev);
> +}
> +#endif
>  
>  static struct pci_driver ahci_pci_driver = {
>  	.name			= DRV_NAME,
>  	.id_table		= ahci_pci_tbl,
>  	.probe			= ahci_init_one,
> +#ifdef CONFIG_SATA_ZPODD
> +	.remove			= ahci_remove_one,
> +#else
>  	.remove			= ata_pci_remove_one,
> +#endif
>  #ifdef CONFIG_PM
>  	.suspend		= ahci_pci_device_suspend,
>  	.resume			= ahci_pci_device_resume,
> diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
> index 51af275..87e4e6d 100644
> --- a/drivers/ata/ahci.h
> +++ b/drivers/ata/ahci.h
> @@ -383,6 +383,10 @@ void ahci_print_info(struct ata_host *host, const char *scc_s);
>  int ahci_host_activate(struct ata_host *host, int irq, unsigned int n_msis);
>  void ahci_error_handler(struct ata_port *ap);
>  
> +#ifdef CONFIG_SATA_ZPODD
> +extern void zpodd_exit(struct ata_device *dev);
> +#endif /* CONFIG_SATA_ZPODD */
> +
>  static inline void __iomem *__ahci_port_base(struct ata_host *host,
>  					     unsigned int port_no)
>  {
> diff --git a/drivers/ata/libata-zpodd.c b/drivers/ata/libata-zpodd.c
> index f3a65a3..fe66949 100644
> --- a/drivers/ata/libata-zpodd.c
> +++ b/drivers/ata/libata-zpodd.c
> @@ -281,3 +281,4 @@ void zpodd_exit(struct ata_device *dev)
>  	kfree(dev->zpodd);
>  	dev->zpodd = NULL;
>  }
> +EXPORT_SYMBOL_GPL(zpodd_exit);

Then you would also not need to export zpodd_exit()..

Best regards,
--
Bartlomiej Zolnierkiewicz
Samsung R&D Institute Poland
Samsung Electronics