| Message ID | 20240306012226.3398927-5-ipylypiv@google.com |
|---|---|
| State | New |
| Headers | show |
| Series | NCQ Priority sysfs sttributes for libsas | expand |
On Tue, Mar 05, 2024 at 05:22:23PM -0800, Igor Pylypiv wrote: > The added sysfs attributes group enables the configuration of NCQ Priority > feature for HBAs that rely on libsas to manage SATA devices. > > Reviewed-by: John Garry <john.g.garry@oracle.com> > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > Reviewed-by: Jason Yan <yanaijie@huawei.com> > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > --- > drivers/scsi/mvsas/mv_init.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > index 43ebb331e216..f1090bb5f2c9 100644 > --- a/drivers/scsi/mvsas/mv_init.c > +++ b/drivers/scsi/mvsas/mv_init.c > @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = { > }; > > static const struct attribute_group *mvst_host_groups[]; > +static const struct attribute_group *mvst_sdev_groups[]; I think you can remove this line. > > #define SOC_SAS_NUM 2 > > @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = { > .compat_ioctl = sas_ioctl, > #endif > .shost_groups = mvst_host_groups, > + .sdev_groups = mvst_sdev_groups, > .track_queue_depth = 1, > }; > > @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = { > > ATTRIBUTE_GROUPS(mvst_host); > > +static const struct attribute_group *mvst_sdev_groups[] = { > + &sas_ata_sdev_attr_group, > + NULL > +}; ..and move these lines up to be after: static const struct attribute_group *mvst_host_groups[]; > + > module_init(mvs_init); > module_exit(mvs_exit); > > -- > 2.44.0.278.ge034bb2e1d-goog >
On Wed, Mar 06, 2024 at 11:55:19AM +0100, Niklas Cassel wrote: > On Tue, Mar 05, 2024 at 05:22:23PM -0800, Igor Pylypiv wrote: > > The added sysfs attributes group enables the configuration of NCQ Priority > > feature for HBAs that rely on libsas to manage SATA devices. > > > > Reviewed-by: John Garry <john.g.garry@oracle.com> > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > > Reviewed-by: Jason Yan <yanaijie@huawei.com> > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > --- > > drivers/scsi/mvsas/mv_init.c | 7 +++++++ > > 1 file changed, 7 insertions(+) > > > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > > index 43ebb331e216..f1090bb5f2c9 100644 > > --- a/drivers/scsi/mvsas/mv_init.c > > +++ b/drivers/scsi/mvsas/mv_init.c > > @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = { > > }; > > > > static const struct attribute_group *mvst_host_groups[]; > > +static const struct attribute_group *mvst_sdev_groups[]; > > I think you can remove this line. > I kept the forward declaration to match the mvst_host_groups style. Perhaps mvs_sht can be moved to the bottom of the file so that all forward declarations can be removed? This can be done in a separate cleanup patch series. I'll keep this and aic94xx patches as-is, unless there are objections. > > > > > #define SOC_SAS_NUM 2 > > > > @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = { > > .compat_ioctl = sas_ioctl, > > #endif > > .shost_groups = mvst_host_groups, > > + .sdev_groups = mvst_sdev_groups, > > .track_queue_depth = 1, > > }; > > > > @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = { > > > > ATTRIBUTE_GROUPS(mvst_host); > > > > +static const struct attribute_group *mvst_sdev_groups[] = { > > + &sas_ata_sdev_attr_group, > > + NULL > > +}; > > ..and move these lines up to be after: > static const struct attribute_group *mvst_host_groups[]; > > > > + > > module_init(mvs_init); > > module_exit(mvs_exit); > > > > -- > > 2.44.0.278.ge034bb2e1d-goog > >
On Wed, Mar 06, 2024 at 01:13:22PM -0800, Igor Pylypiv wrote: > On Wed, Mar 06, 2024 at 11:55:19AM +0100, Niklas Cassel wrote: > > On Tue, Mar 05, 2024 at 05:22:23PM -0800, Igor Pylypiv wrote: > > > The added sysfs attributes group enables the configuration of NCQ Priority > > > feature for HBAs that rely on libsas to manage SATA devices. > > > > > > Reviewed-by: John Garry <john.g.garry@oracle.com> > > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > > > Reviewed-by: Jason Yan <yanaijie@huawei.com> > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > > --- > > > drivers/scsi/mvsas/mv_init.c | 7 +++++++ > > > 1 file changed, 7 insertions(+) > > > > > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > > > index 43ebb331e216..f1090bb5f2c9 100644 > > > --- a/drivers/scsi/mvsas/mv_init.c > > > +++ b/drivers/scsi/mvsas/mv_init.c > > > @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = { > > > }; > > > > > > static const struct attribute_group *mvst_host_groups[]; > > > +static const struct attribute_group *mvst_sdev_groups[]; > > > > I think you can remove this line. > > > I kept the forward declaration to match the mvst_host_groups style. > > Perhaps mvs_sht can be moved to the bottom of the file so that all forward > declarations can be removed? This can be done in a separate cleanup patch > series. > > I'll keep this and aic94xx patches as-is, unless there are objections. Usually, you first do the cleanup, then you do your changes. (That way, there are fewer lines changed, since each patch is smaller.) But no objection from me. Kind regards, Niklas > > > > > > > > > #define SOC_SAS_NUM 2 > > > > > > @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = { > > > .compat_ioctl = sas_ioctl, > > > #endif > > > .shost_groups = mvst_host_groups, > > > + .sdev_groups = mvst_sdev_groups, > > > .track_queue_depth = 1, > > > }; > > > > > > @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = { > > > > > > ATTRIBUTE_GROUPS(mvst_host); > > > > > > +static const struct attribute_group *mvst_sdev_groups[] = { > > > + &sas_ata_sdev_attr_group, > > > + NULL > > > +}; > > > > ..and move these lines up to be after: > > static const struct attribute_group *mvst_host_groups[]; > > > > > > > + > > > module_init(mvs_init); > > > module_exit(mvs_exit); > > > > > > -- > > > 2.44.0.278.ge034bb2e1d-goog > > >
On Thu, Mar 07, 2024 at 10:52:08AM +0100, Niklas Cassel wrote: > On Wed, Mar 06, 2024 at 01:13:22PM -0800, Igor Pylypiv wrote: > > On Wed, Mar 06, 2024 at 11:55:19AM +0100, Niklas Cassel wrote: > > > On Tue, Mar 05, 2024 at 05:22:23PM -0800, Igor Pylypiv wrote: > > > > The added sysfs attributes group enables the configuration of NCQ Priority > > > > feature for HBAs that rely on libsas to manage SATA devices. > > > > > > > > Reviewed-by: John Garry <john.g.garry@oracle.com> > > > > Reviewed-by: Damien Le Moal <dlemoal@kernel.org> > > > > Reviewed-by: Jason Yan <yanaijie@huawei.com> > > > > Signed-off-by: Igor Pylypiv <ipylypiv@google.com> > > > > --- > > > > drivers/scsi/mvsas/mv_init.c | 7 +++++++ > > > > 1 file changed, 7 insertions(+) > > > > > > > > diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c > > > > index 43ebb331e216..f1090bb5f2c9 100644 > > > > --- a/drivers/scsi/mvsas/mv_init.c > > > > +++ b/drivers/scsi/mvsas/mv_init.c > > > > @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = { > > > > }; > > > > > > > > static const struct attribute_group *mvst_host_groups[]; > > > > +static const struct attribute_group *mvst_sdev_groups[]; > > > > > > I think you can remove this line. > > > > > I kept the forward declaration to match the mvst_host_groups style. > > > > Perhaps mvs_sht can be moved to the bottom of the file so that all forward > > declarations can be removed? This can be done in a separate cleanup patch > > series. > > > > I'll keep this and aic94xx patches as-is, unless there are objections. > > Usually, you first do the cleanup, then you do your changes. > (That way, there are fewer lines changed, since each patch is smaller.) > Ack. I think it makes sense to wait for the John's patch series "Add LIBSAS_SHT_BASE for libsas" to get merged first: https://lore.kernel.org/linux-scsi/20240305122452.340471-1-john.g.garry@oracle.com/T/#t This way John woudn't need to re-do the patches on top of the moved sht. Since the LIBSAS_SHT_BASE reduces the line count the following cleanup patches will have fewer lines changed. > But no objection from me. > > > Kind regards, > Niklas > > > > > > > > > > > > > > > #define SOC_SAS_NUM 2 > > > > > > > > @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = { > > > > .compat_ioctl = sas_ioctl, > > > > #endif > > > > .shost_groups = mvst_host_groups, > > > > + .sdev_groups = mvst_sdev_groups, > > > > .track_queue_depth = 1, > > > > }; > > > > > > > > @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = { > > > > > > > > ATTRIBUTE_GROUPS(mvst_host); > > > > > > > > +static const struct attribute_group *mvst_sdev_groups[] = { > > > > + &sas_ata_sdev_attr_group, > > > > + NULL > > > > +}; > > > > > > ..and move these lines up to be after: > > > static const struct attribute_group *mvst_host_groups[]; > > > > > > > > > > + > > > > module_init(mvs_init); > > > > module_exit(mvs_exit); > > > > > > > > -- > > > > 2.44.0.278.ge034bb2e1d-goog > > > >
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c index 43ebb331e216..f1090bb5f2c9 100644 --- a/drivers/scsi/mvsas/mv_init.c +++ b/drivers/scsi/mvsas/mv_init.c @@ -26,6 +26,7 @@ static const struct mvs_chip_info mvs_chips[] = { }; static const struct attribute_group *mvst_host_groups[]; +static const struct attribute_group *mvst_sdev_groups[]; #define SOC_SAS_NUM 2 @@ -53,6 +54,7 @@ static const struct scsi_host_template mvs_sht = { .compat_ioctl = sas_ioctl, #endif .shost_groups = mvst_host_groups, + .sdev_groups = mvst_sdev_groups, .track_queue_depth = 1, }; @@ -779,6 +781,11 @@ static struct attribute *mvst_host_attrs[] = { ATTRIBUTE_GROUPS(mvst_host); +static const struct attribute_group *mvst_sdev_groups[] = { + &sas_ata_sdev_attr_group, + NULL +}; + module_init(mvs_init); module_exit(mvs_exit);