On 2024/09/03 17:01, Niklas Cassel wrote:
> On Tue, Sep 03, 2024 at 07:55:05AM +0900, Damien Le Moal wrote:
>> On 9/3/24 06:04, Niklas Cassel wrote:
>>>> @@ -6000,11 +6018,19 @@ static void ata_port_detach(struct ata_port *ap)
>>>> ata_port_wait_eh(ap);
>>>>
>>>> mutex_lock(&ap->scsi_scan_mutex);
>>>> +
>>>> + /* Cleanup CDL device resources */
>>>> + ata_for_each_link(link, ap, HOST_FIRST) {
>>>> + ata_for_each_dev(dev, link, ALL)
>>>> + ata_dev_cleanup_cdl_resources(dev);
>>>
>>> Here you clean up resources.
>>> Why?
>>> Resources will get cleaned up when ata_port_free() is called,
>>> which will be called by ata_devres_release() -> ata_host_put()
>>> -> ata_host_release() -> ata_port_free(), when the device is
>>> removed.
>>
>> That happens only if the host (=port) is removed, but not if only the device is
>> being removed, e.g. because it was yanked out of its slot (hotplug) or the user
>> removed it using sysfs. In such case, the port remains and is not deleted, so
>> ata_port_free() is not called. Ports exists for as long as the adapter exist.
>>
>>> I don't see any reason to free it here as well.
>>
>> Hotplug of drives :)
>
> ?
>
> This code is in ata_port_detach(), which is called only by ata_host_detach().
>
> So the code you have added here is only when the port is detached, not when
> the device is detached / hotplug removed.
Indeed. I screwed-up with this one :)
> For hotplug it is these functions:
> ata_eh_detach_dev() and ata_scsi_handle_link_detach() that will be called.
And that's the path I should have modified.
> We know that the struct ata_device is never freed until the port is freed,
> but if you want to be nice and free the device's resources on hotplug,
> then I think it is better to remove the freeing for the resources in
> ata_port_free(), and only perform it in ata_eh_dev_disable(), since
> ata_eh_dev_disable() is called both on hotplug removals (by ata_eh_detach_dev())
> and on unload (by ata_eh_unload()).
>
> Unload is performed by ata_port_detach(), so ata_eh_unload() ->
> ata_eh_dev_disable() will always be called, there is no need for an additional
> freeing in ata_port_detach() (or in ata_port_free()).
>
> I suggest something like the following on top of your v4:
> diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c
> index afc245b2f7cc..57e40987b10a 100644
> --- a/drivers/ata/libata-core.c
> +++ b/drivers/ata/libata-core.c
> @@ -5463,11 +5463,6 @@ void ata_port_free(struct ata_port *ap)
> if (!ap)
> return;
>
> - ata_for_each_link(link, ap, HOST_FIRST) {
> - ata_for_each_dev(dev, link, ALL)
> - ata_dev_cleanup_cdl_resources(dev);
> - }
> -
> kfree(ap->pmp_link);
> kfree(ap->slave_link);
> ida_free(&ata_ida, ap->print_id);
> @@ -6019,12 +6014,6 @@ static void ata_port_detach(struct ata_port *ap)
>
> mutex_lock(&ap->scsi_scan_mutex);
>
> - /* Cleanup CDL device resources */
> - ata_for_each_link(link, ap, HOST_FIRST) {
> - ata_for_each_dev(dev, link, ALL)
> - ata_dev_cleanup_cdl_resources(dev);
> - }
> -
> spin_lock_irqsave(ap->lock, flags);
>
> /* Remove scsi devices */
> @@ -6055,13 +6044,6 @@ static void ata_port_detach(struct ata_port *ap)
> cancel_delayed_work_sync(&ap->hotplug_task);
> cancel_delayed_work_sync(&ap->scsi_rescan_task);
>
> - /* clean up zpodd on port removal */
> - ata_for_each_link(link, ap, HOST_FIRST) {
> - ata_for_each_dev(dev, link, ALL) {
> - if (zpodd_dev_enabled(dev))
> - zpodd_exit(dev);
> - }
> - }
> if (ap->pmp_link) {
> int i;
> for (i = 0; i < SATA_PMP_MAX_PORTS; i++)
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 41f1bee0b434..9e870e01509d 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -494,13 +494,21 @@ void ata_eh_release(struct ata_port *ap)
> mutex_unlock(&ap->host->eh_mutex);
> }
>
> +static void ata_eh_dev_free_resources(struct ata_device *dev)
> +{
> + if (zpodd_dev_enabled(dev))
> + zpodd_exit(dev);
> +
> + ata_dev_cleanup_cdl_resources(dev);
> +}
> +
> static void ata_eh_dev_disable(struct ata_device *dev)
> {
> ata_acpi_on_disable(dev);
> ata_down_xfermask_limit(dev, ATA_DNXFER_FORCE_PIO0 | ATA_DNXFER_QUIET);
> dev->class++;
>
> - ata_dev_cleanup_cdl_resources(dev);
> + ata_eh_dev_free_resources(dev);
>
> /* From now till the next successful probe, ering is used to
> * track probe failures. Clear accumulated device error info.
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index 3407e764a5ff..8cd241d39c14 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -4612,9 +4612,6 @@ static void ata_scsi_handle_link_detach(struct ata_link *link)
> dev->flags &= ~ATA_DFLAG_DETACHED;
> spin_unlock_irqrestore(ap->lock, flags);
>
> - if (zpodd_dev_enabled(dev))
> - zpodd_exit(dev);
> -
> ata_scsi_remove_dev(dev);
> }
> }
>
>
> So that we keep all the freeing of struct ata_device's resources in a single
> function.
Looks better. I will integrate that in patch 7. Thanks.
>
>
> Kind regards,
> Niklas
--
Damien Le Moal
Western Digital Research
