| Message ID | 20230720004257.307031-4-nks@flawful.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | libata: remove references to 'old' error handler | expand |
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) > > 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. > > > { > > - int rc = ap->ops->port_start(ap); > > I am not sure how this change is really relevant to " Is > (ata_sas_port_destroy()) now a wrapper around kfree(), so call it directly." Agreed, I will move it to the previous patch, so we also avoid the null pointer dereference in the previous patch. > > > - > > - if (rc) > > - return rc; > > ap->print_id = atomic_inc_return(&ata_print_id); > > return 0; > > always returns 0, so pretty pointless to return a value at all Yes, that would be a small optimization, but I would consider such a change outside the scope of this series. Kind regards, Niklas > > > } > > @@ -1198,20 +1194,6 @@ void ata_sas_tport_delete(struct ata_port *ap) > > } > > EXPORT_SYMBOL_GPL(ata_sas_tport_delete); > > -/** > > - * ata_sas_port_destroy - Destroy a SATA port allocated by ata_sas_port_alloc > > - * @ap: SATA port to destroy > > - * > > - */ > > - > > -void ata_sas_port_destroy(struct ata_port *ap) > > -{ > > - if (ap->ops->port_stop) > > - ap->ops->port_stop(ap); > > - kfree(ap); > > -} > > -EXPORT_SYMBOL_GPL(ata_sas_port_destroy); > > - > > /** > > * ata_sas_slave_configure - Default slave_config routine for libata devices > > * @sdev: SCSI device to configure > > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c > > index 7ead1f1be97f..a2eb9a2191c0 100644 > > --- a/drivers/scsi/libsas/sas_ata.c > > +++ b/drivers/scsi/libsas/sas_ata.c > > @@ -619,7 +619,7 @@ int sas_ata_init(struct domain_device *found_dev) > > return 0; > > destroy_port: > > - ata_sas_port_destroy(ap); > > + kfree(ap); > > free_host: > > ata_host_put(ata_host); > > return rc; > > diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c > > index 8c6afe724944..07e18cdb85c7 100644 > > --- a/drivers/scsi/libsas/sas_discover.c > > +++ b/drivers/scsi/libsas/sas_discover.c > > @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref) > > if (dev_is_sata(dev) && dev->sata_dev.ap) { > > ata_sas_tport_delete(dev->sata_dev.ap); > > - ata_sas_port_destroy(dev->sata_dev.ap); > > + kfree(dev->sata_dev.ap); > > ata_host_put(dev->sata_dev.ata_host); > > dev->sata_dev.ata_host = NULL; > > dev->sata_dev.ap = NULL; > > diff --git a/include/linux/libata.h b/include/linux/libata.h > > index 9424c490ef0b..53cfb1a4b97a 100644 > > --- a/include/linux/libata.h > > +++ b/include/linux/libata.h > > @@ -1238,7 +1238,6 @@ 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 void ata_sas_port_destroy(struct ata_port *); > > extern struct ata_port *ata_sas_port_alloc(struct ata_host *, > > struct ata_port_info *, struct Scsi_Host *); > > extern void ata_sas_async_probe(struct ata_port *ap); >
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
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) { - int rc = ap->ops->port_start(ap); - - if (rc) - return rc; ap->print_id = atomic_inc_return(&ata_print_id); return 0; } @@ -1198,20 +1194,6 @@ void ata_sas_tport_delete(struct ata_port *ap) } EXPORT_SYMBOL_GPL(ata_sas_tport_delete); -/** - * ata_sas_port_destroy - Destroy a SATA port allocated by ata_sas_port_alloc - * @ap: SATA port to destroy - * - */ - -void ata_sas_port_destroy(struct ata_port *ap) -{ - if (ap->ops->port_stop) - ap->ops->port_stop(ap); - kfree(ap); -} -EXPORT_SYMBOL_GPL(ata_sas_port_destroy); - /** * ata_sas_slave_configure - Default slave_config routine for libata devices * @sdev: SCSI device to configure diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c index 7ead1f1be97f..a2eb9a2191c0 100644 --- a/drivers/scsi/libsas/sas_ata.c +++ b/drivers/scsi/libsas/sas_ata.c @@ -619,7 +619,7 @@ int sas_ata_init(struct domain_device *found_dev) return 0; destroy_port: - ata_sas_port_destroy(ap); + kfree(ap); free_host: ata_host_put(ata_host); return rc; diff --git a/drivers/scsi/libsas/sas_discover.c b/drivers/scsi/libsas/sas_discover.c index 8c6afe724944..07e18cdb85c7 100644 --- a/drivers/scsi/libsas/sas_discover.c +++ b/drivers/scsi/libsas/sas_discover.c @@ -301,7 +301,7 @@ void sas_free_device(struct kref *kref) if (dev_is_sata(dev) && dev->sata_dev.ap) { ata_sas_tport_delete(dev->sata_dev.ap); - ata_sas_port_destroy(dev->sata_dev.ap); + kfree(dev->sata_dev.ap); ata_host_put(dev->sata_dev.ata_host); dev->sata_dev.ata_host = NULL; dev->sata_dev.ap = NULL; diff --git a/include/linux/libata.h b/include/linux/libata.h index 9424c490ef0b..53cfb1a4b97a 100644 --- a/include/linux/libata.h +++ b/include/linux/libata.h @@ -1238,7 +1238,6 @@ 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 void ata_sas_port_destroy(struct ata_port *); extern struct ata_port *ata_sas_port_alloc(struct ata_host *, struct ata_port_info *, struct Scsi_Host *); extern void ata_sas_async_probe(struct ata_port *ap);