Re: [PATCH v2 12/13] ata,scsi: Remove useless ata_sas_port_alloc() wrapper

[Niklas Cassel <cassel@kernel.org>]

On Thu, Jun 27, 2024 at 10:46:11AM +0900, Damien Le Moal wrote:
> On 6/27/24 03:00, Niklas Cassel wrote:
> > Now when the ap->print_id assignment has moved to ata_port_alloc(),
> > we can remove the useless ata_sas_port_alloc() wrapper.
> 
> Same comment as for patch 4: not a fan.
> 
> But I do like the fact that the port additional initialization is moved to
> libsas, as that code is completely dependent on libsas.
> 
> What about this cleanup, which would make more sense:
> 
> 1) Keep the ata_sas_xxx() exported symbols, even if they are trivial.
> 2) Move all these wrappers to a new file (libata-sas.c) and make this file
> compilation dependend on CONFIG_SATA_HOST and CONFIG_SCSI_SAS_LIBSAS.
> 
> That has the benefit of keeping all the libsas wrappers together and to reduce
> the binary size for configs that do not enable libsas.
> 
> Thoughts ?

I think that:

1) These wrappers are like a virus... they are completely useless and
   having them will force us to keep adding new wrappers, e.g. we would
   need to add a new wrapper for ata_port_free().

2) Having a wrapper that simply does an EXPORT_SYMBOL is not only useless,
it also makes it harder to know that the function (called by the wrapper)
is actually non-internal, since the function will be defined in the libata
internal header in drivers/ata/libata.h, so you might think that it is an
internal function... but it isn't, since there is a wrapper exporting it :)

3) The naming prefix argument does not hold up.
   If you do a:

$ git grep -E "\s+ata_\S+\(" v6.10-rc5 drivers/scsi/libsas/
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_qc_complete(qc);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_to_fis(&qc->tf, qc->dev->link->pmp, 1, (u8 *)&task->ata_task.fis);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        task->ata_task.use_ncq = ata_is_ncq(qc->tf.protocol);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        task->ata_task.dma_xfer = ata_is_dma(qc->tf.protocol);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_from_fis(dev->sata_dev.fis, &qc->result_tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_tf_from_fis(dev->frame_rcvd, &tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        return ata_dev_classify(&tf);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ret = ata_wait_after_reset(link, deadline, check_ready);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_std_sched_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_host_init(ata_host, ha->dev, &sas_sata_ops);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_sas_tport_add(ata_host->dev, ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_host_put(ata_host);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_port_probe(dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_sas_port_suspend(sata->ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                ata_sas_port_resume(sata->ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_scsi_port_error_handler(ha->shost, ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                        ata_scsi_cmd_error_handler(shost, ap, &sata_q);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:                         * action will be ata_port_error_handler()
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_port_schedule_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_port_wait_eh(ap);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        ata_link_abort(link);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_supported(ddev->sata_dev.ap, sdev, &supported);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_enabled(ddev->sata_dev.ap, sdev, &enabled);
v6.10-rc5:drivers/scsi/libsas/sas_ata.c:        rc = ata_ncq_prio_enable(ddev->sata_dev.ap, sdev, enable);
v6.10-rc5:drivers/scsi/libsas/sas_discover.c:           ata_sas_tport_delete(dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_discover.c:           ata_host_put(dev->sata_dev.ata_host);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          res = ata_sas_queuecmd(cmd, dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          return ata_sas_scsi_ioctl(dev->sata_dev.ap, sdev, cmd, arg);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          ata_sas_device_configure(scsi_dev, lim, dev->sata_dev.ap);
v6.10-rc5:drivers/scsi/libsas/sas_scsi_host.c:          return ata_change_queue_depth(dev->sata_dev.ap, sdev, depth);

You will see that far from all libata functions have ata_sas_*() wrapper,
so all the ata_* functions that do not not have ata_sas_*() wrapper are
already exported using EXPORT_SYMBOL_GPL(), i.e.:

ata_qc_complete(), ata_tf_to_fis(), ata_tf_from_fis(), ata_dev_classify(),
ata_wait_after_reset(), ata_std_sched_eh(), ata_host_init(), ata_host_put(),
ata_port_probe(), ata_scsi_port_error_handler(), ata_scsi_cmd_error_handler(),
ata_port_schedule_eh(), ata_link_abort(), ata_ncq_prio_supported(),
ata_ncq_prio_enabled(), ata_ncq_prio_enable(), ata_change_queue_depth()
are already exported using EXPORT_SYMBOL_GPL().

(And yes, some of these exported functions not used by any libata SATA driver
(compiled as a separate .ko) other than libsas.)


TL;DR: I really think that we should kill these wrappers.


Kind regards,
Niklas