diff mbox series

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

Message ID 20240626180031.4050226-27-cassel@kernel.org
State Superseded
Headers show
Series ata,libsas: Assign the unique id used for printing earlier | expand

Commit Message

Niklas Cassel June 26, 2024, 6 p.m. UTC
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(-)

Comments

Damien Le Moal June 27, 2024, 1:46 a.m. UTC | #1
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 ?
Hannes Reinecke June 27, 2024, 6:40 a.m. UTC | #2
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
Niklas Cassel June 27, 2024, 9:48 a.m. UTC | #3
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
Damien Le Moal June 28, 2024, 3:46 a.m. UTC | #4
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 mbox series

Patch

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);