Re: [PATCH v2 3/8] ata,scsi: remove ata_sas_port_destroy()

[John Garry <john.g.garry@oracle.com>]

On 21/07/2023 14:33, Niklas Cassel wrote:
> On Thu, Jul 20, 2023 at 09:57:29AM +0100, John Garry wrote:
>> On 20/07/2023 01:42, Niklas Cassel wrote:
>>> From: Hannes Reinecke <hare@suse.de>
>>>
>>> Is now a wrapper around kfree(), so call it directly.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> Signed-off-by: Niklas Cassel <niklas.cassel@wdc.com>
>>> ---
>>>    drivers/ata/libata-sata.c          | 18 ------------------
>>>    drivers/scsi/libsas/sas_ata.c      |  2 +-
>>>    drivers/scsi/libsas/sas_discover.c |  2 +-
>>>    include/linux/libata.h             |  1 -
>>>    4 files changed, 2 insertions(+), 21 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c
>>> index d3b595294eee..b5de0f40ea25 100644
>>> --- a/drivers/ata/libata-sata.c
>>> +++ b/drivers/ata/libata-sata.c
>>> @@ -1177,10 +1177,6 @@ EXPORT_SYMBOL_GPL(ata_sas_sync_probe);
>>>    int ata_sas_port_init(struct ata_port *ap)
>>

Hi Niklas,

>> This is a bit of a daft function now, considering it only does
>> atomic_inc_return(&ata_print_id). Do we really need to export a symbol for
>> that?
> 
> $ git grep ata_print_id
> drivers/ata/libata-core.c:atomic_t ata_print_id = ATOMIC_INIT(0);
> drivers/ata/libata-core.c:              host->ports[i]->print_id = atomic_inc_return(&ata_print_id);
> drivers/ata/libata-sata.c:      ap->print_id = atomic_inc_return(&ata_print_id);
> drivers/ata/libata.h:extern atomic_t ata_print_id;
> 
> It seems to be defined and used only in libata, while I agree that the function
> is a bit silly, with my limited knowledge of how the linker works, moving it to
> libsas seems a bit dangerous...
> 
> You can build libata as a module and libsas as built-in, and vice versa...
> 
> Also, since there are no direct users in libsas, I'd rather keep it in libata.

I wasn't really suggesting to move it to libsas - indeed, it is libata 
functionality.

Could you just put the ap->print_id = atomic_inc_return(&ata_print_id) 
call in ata_sas_port_alloc() (and remove ata_sas_port_init())? 
ata_sas_port_alloc() is only called from libsas, and ata_sas_port_init() 
is called straight after ata_sas_port_alloc() there. And 
ata_sas_port_alloc() is already doing ap init also (so setting 
ap->print_id would not be out of place there).

Thanks,
John