| Message ID | 20240308114339.1340549-1-john.g.garry@oracle.com |
|---|---|
| Headers | show |
| Series | Add LIBSAS_SHT_BASE for libsas | expand |
On Fri, Mar 08, 2024 at 11:43:34AM +0000, John Garry wrote: > There is much duplication in the scsi_host_template structure for the > drivers which use libsas. > > Similar to how a standard template is used in libata with __ATA_BASE_SHT, > create a standard template in LIBSAS_SHT_BASE. > > Don't set a default for max_sectors at SCSI_DEFAULT_MAX_SECTORS, as > scsi_host_alloc() will default to this value automatically. > > Even though some drivers don't set proc_name, it won't make much difference > to set as DRV_NAME. > > Also add LIBSAS_SHT_BASE_NO_SLAVE_INIT for the hisi_sas drivers which have > custom .slave_alloc and .slave_configure methods. Looks like libata drivers have no problem overriding default values that were set by __ATA_BASE_SHT. For example __ATA_BASE_SHT sets .can_queue .sdev_attrs and then AHCI_SHT overrides those with AHCI specific values: #define AHCI_SHT(drv_name) \ ATA_NCQ_SHT(drv_name), \ .can_queue = AHCI_MAX_CMDS, \ .sg_tablesize = AHCI_MAX_SG, \ .dma_boundary = AHCI_DMA_BOUNDARY, \ .shost_attrs = ahci_shost_attrs, \ .sdev_attrs = ahci_sdev_attrs Perhaps there is no need for LIBSAS_SHT_BASE_NO_SLAVE_INIT since hisi_sas can use LIBSAS_SHT_BASE with .slave_alloc and .slave_configure overrides? Similarly hisi_sas and pm8001 can override the default ".sg_tablesize = SG_ALL". Thanks, Igor > > Reviewed-by: Jason Yan <yanaijie@huawei.com> > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > include/scsi/libsas.h | 29 +++++++++++++++++++++++++++++ > 1 file changed, 29 insertions(+) > > diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h > index f5257103fdb6..de842602f47e 100644 > --- a/include/scsi/libsas.h > +++ b/include/scsi/libsas.h > @@ -726,4 +726,33 @@ void sas_notify_port_event(struct asd_sas_phy *phy, enum port_event event, > void sas_notify_phy_event(struct asd_sas_phy *phy, enum phy_event event, > gfp_t gfp_flags); > > +#define __LIBSAS_SHT_BASE \ > + .module = THIS_MODULE, \ > + .name = DRV_NAME, \ > + .proc_name = DRV_NAME, \ > + .queuecommand = sas_queuecommand, \ > + .dma_need_drain = ata_scsi_dma_need_drain, \ > + .target_alloc = sas_target_alloc, \ > + .change_queue_depth = sas_change_queue_depth, \ > + .bios_param = sas_bios_param, \ > + .this_id = -1, \ > + .eh_device_reset_handler = sas_eh_device_reset_handler, \ > + .eh_target_reset_handler = sas_eh_target_reset_handler, \ > + .target_destroy = sas_target_destroy, \ > + .ioctl = sas_ioctl, \ > + > +#ifdef CONFIG_COMPAT > +#define _LIBSAS_SHT_BASE __LIBSAS_SHT_BASE \ > + .compat_ioctl = sas_ioctl, > +#else > +#define _LIBSAS_SHT_BASE __LIBSAS_SHT_BASE > +#endif > + > +#define LIBSAS_SHT_BASE _LIBSAS_SHT_BASE \ > + .slave_configure = sas_slave_configure, \ > + .slave_alloc = sas_slave_alloc, \ > + > +#define LIBSAS_SHT_BASE_NO_SLAVE_INIT _LIBSAS_SHT_BASE > + > + > #endif /* _SASLIB_H_ */ > -- > 2.31.1 >
On 08/03/2024 19:41, Igor Pylypiv wrote: >> Even though some drivers don't set proc_name, it won't make much difference >> to set as DRV_NAME. >> >> Also add LIBSAS_SHT_BASE_NO_SLAVE_INIT for the hisi_sas drivers which have >> custom .slave_alloc and .slave_configure methods. > Looks like libata drivers have no problem overriding default values that were > set by __ATA_BASE_SHT. For example __ATA_BASE_SHT sets .can_queue .sdev_attrs > and then AHCI_SHT overrides those with AHCI specific values: > > #define AHCI_SHT(drv_name) \ > ATA_NCQ_SHT(drv_name), \ which tag are you looking at here? That looks like an old definition of AHCI_SHT(). There was a significant change for this in the following: https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/ata/ahci.h?h=v5.14&id=071e86fe2872e7442e42ad26f71cd6bde55344f8 Thanks, John
On Sun, Mar 10, 2024 at 10:02:42AM +0000, John Garry wrote: > On 08/03/2024 19:41, Igor Pylypiv wrote: > > > Even though some drivers don't set proc_name, it won't make much difference > > > to set as DRV_NAME. > > > > > > Also add LIBSAS_SHT_BASE_NO_SLAVE_INIT for the hisi_sas drivers which have > > > custom .slave_alloc and .slave_configure methods. > > Looks like libata drivers have no problem overriding default values that were > > set by __ATA_BASE_SHT. For example __ATA_BASE_SHT sets .can_queue .sdev_attrs > > and then AHCI_SHT overrides those with AHCI specific values: > > > > #define AHCI_SHT(drv_name) \ > > ATA_NCQ_SHT(drv_name), \ > > which tag are you looking at here? > > That looks like an old definition of AHCI_SHT(). > > There was a significant change for this in the following: > https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/drivers/ata/ahci.h?h=v5.14&id=071e86fe2872e7442e42ad26f71cd6bde55344f8 Oh, my bad. I had some old kernel version checked out. Please disregard. Reviewed-by: Igor Pylypiv <ipylypiv@google.com> Thanks, Igor > > Thanks, > John
On Fri, Mar 08, 2024 at 11:43:35AM +0000, John Garry wrote: > Use standard template for scsi_host_template structure to reduce > duplication. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/pm8001/pm8001_init.c | 20 +------------------- > 1 file changed, 1 insertion(+), 19 deletions(-) Reviewed-by: Igor Pylypiv <ipylypiv@google.com> Thanks, Igor
On Fri, Mar 08, 2024 at 11:43:37AM +0000, John Garry wrote: > Use standard template for scsi_host_template structure to reduce > duplication. > > Reviewed-by: Jason Yan <yanaijie@huawei.com> > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/aic94xx/aic94xx_init.c | 21 ++------------------- > 1 file changed, 2 insertions(+), 19 deletions(-) Reviewed-by: Igor Pylypiv <ipylypiv@google.com> Thanks, Igor
On Fri, Mar 08, 2024 at 11:43:38AM +0000, John Garry wrote: > Use standard template for scsi_host_template structure to reduce > duplication. > > Signed-off-by: John Garry <john.g.garry@oracle.com> > --- > drivers/scsi/mvsas/mv_init.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) Reviewed-by: Igor Pylypiv <ipylypiv@google.com> Thanks, Igor > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > index 43ebb331e216..fb81d267c9cc 100644 > --- a/drivers/scsi/mvsas/mv_init.c > +++ b/drivers/scsi/mvsas/mv_init.c > @@ -30,28 +30,11 @@ static const struct attribute_group *mvst_host_groups[]; > #define SOC_SAS_NUM 2 > > static const struct scsi_host_template mvs_sht = { > - .module = THIS_MODULE, > - .name = DRV_NAME, > - .queuecommand = sas_queuecommand, > - .dma_need_drain = ata_scsi_dma_need_drain, > - .target_alloc = sas_target_alloc, > - .slave_configure = sas_slave_configure, > + LIBSAS_SHT_BASE > .scan_finished = mvs_scan_finished, > .scan_start = mvs_scan_start, > - .change_queue_depth = sas_change_queue_depth, > - .bios_param = sas_bios_param, > .can_queue = 1, > - .this_id = -1, > .sg_tablesize = SG_ALL, > - .max_sectors = SCSI_DEFAULT_MAX_SECTORS, > - .eh_device_reset_handler = sas_eh_device_reset_handler, > - .eh_target_reset_handler = sas_eh_target_reset_handler, > - .slave_alloc = sas_slave_alloc, > - .target_destroy = sas_target_destroy, > - .ioctl = sas_ioctl, > -#ifdef CONFIG_COMPAT > - .compat_ioctl = sas_ioctl, > -#endif > .shost_groups = mvst_host_groups, > .track_queue_depth = 1, > }; > -- > 2.31.1 >
Hi John, On 2024/3/8 19:43, John Garry wrote: > There is much duplication in the scsi_host_template structure for the > drivers which use libsas. > > Similar to how a standard template is used in libata with __ATA_BASE_SHT, > create a standard template in LIBSAS_SHT_BASE. > > Based on following: > b914227e4215 (tag: mkp-scsi-staging, mkp-scsi/staging, mkp-scsi/for-next, mkp-scsi/6.9/scsi-staging) Merge patch series "Pass data lifetime information to SCSI disk devices" > > Differences to v1: > - tidy libsas.h change (Jason) > - Don't set eh_abort_handler in LIBSAS_SHT_BASE (Jason) > - Remove sg_tablesize in LIBSAS_SHT_BASE, as W=1 build dislikes it I cannot find sg_tablesize in LIBSAS_SHT_BASE in v1, did I misssed anything? Thanks, Jason > - Add some RB tags (Thanks) > > John Garry (6): > scsi: libsas: Add LIBSAS_SHT_BASE > scsi: pm8001: Use LIBSAS_SHT_BASE > scsi: hisi_sas: Use LIBSAS_SHT_BASE_NO_SLAVE_INIT > scsi: aic94xx: Use LIBSAS_SHT_BASE > scsi: mvsas: Use LIBSAS_SHT_BASE > scsi: isci: Use LIBSAS_SHT_BASE > > drivers/scsi/aic94xx/aic94xx_init.c | 21 ++----------------- > drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 18 +--------------- > drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 18 +--------------- > drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 18 +--------------- > drivers/scsi/isci/init.c | 23 ++------------------ > drivers/scsi/mvsas/mv_init.c | 19 +---------------- > drivers/scsi/pm8001/pm8001_init.c | 20 +----------------- > include/scsi/libsas.h | 29 ++++++++++++++++++++++++++ > 8 files changed, 38 insertions(+), 128 deletions(-) >
On 2024/3/8 19:43, John Garry wrote: > Use standard template for scsi_host_template structure to reduce > duplication. > > Signed-off-by: John Garry<john.g.garry@oracle.com> > --- > drivers/scsi/mvsas/mv_init.c | 19 +------------------ > 1 file changed, 1 insertion(+), 18 deletions(-) Reviewed-by: Jason Yan <yanaijie@huawei.com>
On 11/03/2024 01:46, Jason Yan wrote: >> - Remove sg_tablesize in LIBSAS_SHT_BASE, as W=1 build dislikes it > > I cannot find sg_tablesize in LIBSAS_SHT_BASE in v1, did I misssed > anything? Ah, I think that I just had that change local but then decided to drop it due to W=1 build issue. Thanks for your eagle eye checking. John
John, > There is much duplication in the scsi_host_template structure for the > drivers which use libsas. > > Similar to how a standard template is used in libata with > __ATA_BASE_SHT, create a standard template in LIBSAS_SHT_BASE. Applied to 6.10/scsi-staging, thanks!