| Message ID | 20240626180031.4050226-27-cassel@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | ata,libsas: Assign the unique id used for printing earlier | expand |
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 ?
On 6/26/24 20: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. > > Signed-off-by: Niklas Cassel <cassel@kernel.org> > --- > drivers/ata/libata-core.c | 1 + > drivers/ata/libata-sata.c | 35 ----------------------------------- > drivers/ata/libata.h | 1 - > drivers/scsi/libsas/sas_ata.c | 10 ++++++++-- > include/linux/libata.h | 3 +-- > 5 files changed, 10 insertions(+), 40 deletions(-) > Reviewed-by: Hannes Reinecke <hare@suse.de> Cheers, Hannes
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
On 6/27/24 6:48 PM, Niklas Cassel wrote: > 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.: OK. I thought things where a little more consistent than that. Let's kill the wrappers then. Keep your patch as it is !
diff --git a/drivers/ata/libata-core.c b/drivers/ata/libata-core.c index 846ab99e0cd3..11ecfdea5959 100644 --- a/drivers/ata/libata-core.c +++ b/drivers/ata/libata-core.c @@ -5492,6 +5492,7 @@ struct ata_port *ata_port_alloc(struct ata_host *host) return ap; } +EXPORT_SYMBOL_GPL(ata_port_alloc); void ata_port_free(struct ata_port *ap) { diff --git a/drivers/ata/libata-sata.c b/drivers/ata/libata-sata.c index b602247604dc..48660d445602 100644 --- a/drivers/ata/libata-sata.c +++ b/drivers/ata/libata-sata.c @@ -1204,41 +1204,6 @@ int ata_scsi_change_queue_depth(struct scsi_device *sdev, int queue_depth) } EXPORT_SYMBOL_GPL(ata_scsi_change_queue_depth); -/** - * ata_sas_port_alloc - Allocate port for a SAS attached SATA device - * @host: ATA host container for all SAS ports - * @port_info: Information from low-level host driver - * @shost: SCSI host that the scsi device is attached to - * - * LOCKING: - * PCI/etc. bus probe sem. - * - * RETURNS: - * ata_port pointer on success / NULL on failure. - */ - -struct ata_port *ata_sas_port_alloc(struct ata_host *host, - struct ata_port_info *port_info, - struct Scsi_Host *shost) -{ - struct ata_port *ap; - - ap = ata_port_alloc(host); - if (!ap) - return NULL; - - ap->port_no = 0; - ap->pio_mask = port_info->pio_mask; - ap->mwdma_mask = port_info->mwdma_mask; - ap->udma_mask = port_info->udma_mask; - ap->flags |= port_info->flags; - ap->ops = port_info->port_ops; - ap->cbl = ATA_CBL_SATA; - - return ap; -} -EXPORT_SYMBOL_GPL(ata_sas_port_alloc); - /** * ata_sas_device_configure - Default device_configure routine for libata * devices diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h index 38ce13b55474..e930ac948eac 100644 --- a/drivers/ata/libata.h +++ b/drivers/ata/libata.h @@ -82,7 +82,6 @@ extern void ata_link_init(struct ata_port *ap, struct ata_link *link, int pmp); extern int sata_link_init_spd(struct ata_link *link); extern int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg); extern int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg); -extern struct ata_port *ata_port_alloc(struct ata_host *host); extern const char *sata_spd_string(unsigned int spd); extern unsigned int ata_read_log_page(struct ata_device *dev, u8 log, u8 page, void *buf, unsigned int sectors); diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index e8987dce585f..eecdd1525a18 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -597,13 +597,19 @@ int sas_ata_init(struct domain_device *found_dev) ata_host_init(ata_host, ha->dev, &sas_sata_ops); - ap = ata_sas_port_alloc(ata_host, &sata_port_info, shost); + ap = ata_port_alloc(ata_host); if (!ap) { - pr_err("ata_sas_port_alloc failed.\n"); + pr_err("ata_port_alloc failed.\n"); rc = -ENODEV; goto free_host; } + ap->port_no = 0; + ap->pio_mask = sata_port_info.pio_mask; + ap->mwdma_mask = sata_port_info.mwdma_mask; + ap->udma_mask = sata_port_info.udma_mask; + ap->flags |= sata_port_info.flags; + ap->ops = sata_port_info.port_ops; ap->private_data = found_dev; ap->cbl = ATA_CBL_SATA; ap->scsi_host = shost; diff --git a/include/linux/libata.h b/include/linux/libata.h index 84a7bfbac9fa..17394098bee9 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1244,9 +1244,8 @@ extern int sata_link_debounce(struct ata_link *link, extern int sata_link_scr_lpm(struct ata_link *link, enum ata_lpm_policy policy, bool spm_wakeup); extern int ata_slave_link_init(struct ata_port *ap); -extern struct ata_port *ata_sas_port_alloc(struct ata_host *, - struct ata_port_info *, struct Scsi_Host *); extern void ata_port_probe(struct ata_port *ap); +extern struct ata_port *ata_port_alloc(struct ata_host *host); extern void ata_port_free(struct ata_port *ap); extern int ata_tport_add(struct device *parent, struct ata_port *ap); extern void ata_tport_delete(struct ata_port *ap);
Now when the ap->print_id assignment has moved to ata_port_alloc(), we can remove the useless ata_sas_port_alloc() wrapper. Signed-off-by: Niklas Cassel <cassel@kernel.org> --- drivers/ata/libata-core.c | 1 + drivers/ata/libata-sata.c | 35 ----------------------------------- drivers/ata/libata.h | 1 - drivers/scsi/libsas/sas_ata.c | 10 ++++++++-- include/linux/libata.h | 3 +-- 5 files changed, 10 insertions(+), 40 deletions(-)