| Message ID | 20230304003103.2572793-1-bvanassche@acm.org |
|---|---|
| Headers | show |
| Series | Constify most SCSI host templates | expand |
On Fri, Mar 03, 2023 at 04:31:02PM -0800, Bart Van Assche wrote: > Improve source code documentation by constifying host templates that are > not modified. > > Cc: Oliver Neukum <oneukum@suse.com> > Cc: linux-usb@vger.kernel.org > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- For the usb-storage parts: Acked-by: Alan Stern <stern@rowland.harvard.edu> > drivers/usb/image/microtek.c | 2 +- > drivers/usb/storage/uas.c | 2 +- > drivers/usb/storage/usb.c | 2 +- > drivers/usb/storage/usb.h | 2 +- > 4 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/drivers/usb/image/microtek.c b/drivers/usb/image/microtek.c > index 874ea4b54ced..8c8fa71c69c4 100644 > --- a/drivers/usb/image/microtek.c > +++ b/drivers/usb/image/microtek.c > @@ -620,7 +620,7 @@ static int mts_scsi_queuecommand_lck(struct scsi_cmnd *srb) > > static DEF_SCSI_QCMD(mts_scsi_queuecommand) > > -static struct scsi_host_template mts_scsi_host_template = { > +static const struct scsi_host_template mts_scsi_host_template = { > .module = THIS_MODULE, > .name = "microtekX6", > .proc_name = "microtekX6", > diff --git a/drivers/usb/storage/uas.c b/drivers/usb/storage/uas.c > index de3836412bf3..2583ee9815c5 100644 > --- a/drivers/usb/storage/uas.c > +++ b/drivers/usb/storage/uas.c > @@ -894,7 +894,7 @@ static int uas_slave_configure(struct scsi_device *sdev) > return 0; > } > > -static struct scsi_host_template uas_host_template = { > +static const struct scsi_host_template uas_host_template = { > .module = THIS_MODULE, > .name = "uas", > .queuecommand = uas_queuecommand, > diff --git a/drivers/usb/storage/usb.c b/drivers/usb/storage/usb.c > index ed7c6ad96a74..7b36a3334fb3 100644 > --- a/drivers/usb/storage/usb.c > +++ b/drivers/usb/storage/usb.c > @@ -937,7 +937,7 @@ int usb_stor_probe1(struct us_data **pus, > struct usb_interface *intf, > const struct usb_device_id *id, > const struct us_unusual_dev *unusual_dev, > - struct scsi_host_template *sht) > + const struct scsi_host_template *sht) > { > struct Scsi_Host *host; > struct us_data *us; > diff --git a/drivers/usb/storage/usb.h b/drivers/usb/storage/usb.h > index 0451fac1adce..fd3f32670873 100644 > --- a/drivers/usb/storage/usb.h > +++ b/drivers/usb/storage/usb.h > @@ -187,7 +187,7 @@ extern int usb_stor_probe1(struct us_data **pus, > struct usb_interface *intf, > const struct usb_device_id *id, > const struct us_unusual_dev *unusual_dev, > - struct scsi_host_template *sht); > + const struct scsi_host_template *sht); > extern int usb_stor_probe2(struct us_data *us); > extern void usb_stor_disconnect(struct usb_interface *intf); >
On Sat, Mar 4, 2023 at 1:31 AM Bart Van Assche <bvanassche@acm.org> wrote: > Make it explicit that ATA host templates are not modified. > > Cc: Damien Le Moal <damien.lemoal@opensource.wdc.com> > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: John Garry <john.garry@huawei.com> > Cc: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Looks reasonable. Reviewed-by: Linus Walleij <linus.walleij@linaro.org> Yours, Linus Walleij
On 04.03.23 01:31, Bart Van Assche wrote: > Improve source code documentation by constifying host templates that are > not modified. > > Cc: Oliver Neukum <oneukum@suse.com> > Cc: linux-usb@vger.kernel.org > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Acked-by: Oliver Neukum <oneukum@suse.com>
On 04/03/2023 00:29, Bart Van Assche wrote: > Access the qla2xxx_driver_template data structure directly instead of via > the host pointer. This patch prepares for declaring the 'hostt' pointer > const. > > Cc: Nilesh Javali <njavali@marvell.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/qla2xxx/qla_target.c | 4 ++-- Hi Bart, > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c > index aa0cf5ca6c1c..8d9a6aa3ea61 100644 > --- a/drivers/scsi/qla2xxx/qla_target.c > +++ b/drivers/scsi/qla2xxx/qla_target.c > @@ -6395,8 +6395,8 @@ int qlt_add_target(struct qla_hw_data *ha, struct scsi_qla_host *base_vha) > return -ENOMEM; > } > > - if (!(base_vha->host->hostt->supported_mode & MODE_TARGET)) > - base_vha->host->hostt->supported_mode |= MODE_TARGET; > + if (!(qla2xxx_driver_template.supported_mode & MODE_TARGET)) > + qla2xxx_driver_template.supported_mode |= MODE_TARGET; So we're saying if that MODE_TARGET bit is not set, then set it. It would be neater to just always set it, right? Apart from that, I will say that I haven't studied the driver in detail, but my impression is that we should just set this flag per-shost in base_vha->host.active_mode, and not the host template supported_mode member. Indeed, we don't even seem to be making this driver scsi_host_template as const in this series, which I thought was the aim (and I assume because of this). Thanks, John > > rc = btree_init64(&tgt->lun_qpair_map); > if (rc) {
On 3/6/23 05:10, John Garry wrote: > On 04/03/2023 00:29, Bart Van Assche wrote: >> --- a/drivers/scsi/qla2xxx/qla_target.c >> +++ b/drivers/scsi/qla2xxx/qla_target.c >> @@ -6395,8 +6395,8 @@ int qlt_add_target(struct qla_hw_data *ha, >> struct scsi_qla_host *base_vha) >> return -ENOMEM; >> } >> - if (!(base_vha->host->hostt->supported_mode & MODE_TARGET)) >> - base_vha->host->hostt->supported_mode |= MODE_TARGET; >> + if (!(qla2xxx_driver_template.supported_mode & MODE_TARGET)) >> + qla2xxx_driver_template.supported_mode |= MODE_TARGET; > > So we're saying if that MODE_TARGET bit is not set, then set it. It > would be neater to just always set it, right? > > Apart from that, I will say that I haven't studied the driver in detail, > but my impression is that we should just set this flag per-shost in > base_vha->host.active_mode, and not the host template supported_mode > member. Indeed, we don't even seem to be making this driver > scsi_host_template as const in this series, which I thought was the aim > (and I assume because of this). Hi John, If I have to repost this patch series I will implement this suggestion. Thanks, Bart.
On 04/03/2023 00:29, Bart Van Assche wrote: > Prepare for constifying most SCSI host template pointers by constifying > the SCSI host template pointer arguments and variables in the SCSI core. > > Cc: Christoph Hellwig<hch@lst.de> > Cc: Ming Lei<ming.lei@redhat.com> > Cc: Hannes Reinecke<hare@suse.de> > Cc: John Garry<john.garry@huawei.com> > Cc: Mike Christie<michael.christie@oracle.com> > Signed-off-by: Bart Van Assche<bvanassche@acm.org> Hi Bart, You wrote that most pointers were now cast as const - which ones where not? From a quick scan they all seem to be const Apart from that: Reviewed-by: John Garry <john.g.garry@oracle.com> Thanks, John
On 3/6/23 06:29, John Garry wrote: > You wrote that most pointers were now cast as const - which ones where > not? From a quick scan they all seem to be const Hi Garry, Some SCSI drivers modify one of more members of the SCSI host template. An example can be found in drivers/scsi/pcmcia/nsp_cs.c: sht->name = data->nspinfo; Another example from drivers/scsi/bnx2fc/bnx2fc_fcoe.c: bnx2fc_shost_template.can_queue = hba->max_outstanding_cmds; Thanks, Bart.
On 06/03/2023 16:07, Bart Van Assche wrote: > On 3/6/23 06:29, John Garry wrote: >> You wrote that most pointers were now cast as const - which ones were >> not? From a quick scan they all seem to be const > > Hi Garry, > > Some SCSI drivers modify one of more members of the SCSI host template. > An example can be found in drivers/scsi/pcmcia/nsp_cs.c: I seemed to get the wrong idea of what you meant in the commit message. When you wrote "Prepare for constifying most SCSI host template pointers", I got the impression that most of the pointers to SCSI host template in the core code were going to be pointers to const. However you really meant that most of the per-driver SCSI host template instances would be const. Anyway, Reviewed-by: John Garry <john.g.garry@oracle.com> > > sht->name = data->nspinfo; > > Another example from drivers/scsi/bnx2fc/bnx2fc_fcoe.c: > > bnx2fc_shost_template.can_queue = hba->max_outstanding_cmds; BTW, surely we should be setting shost->can_queue = hba->max_outstanding_cmds after scsi_host_alloc() and not modifying bnx2fc_shost_template, right? The series is already huge, so this stuff would be done separately, I suppose. Thanks, John
On 3/6/23 10:55, John Garry wrote: > On 06/03/2023 16:07, Bart Van Assche wrote: >> Another example from drivers/scsi/bnx2fc/bnx2fc_fcoe.c: >> >> bnx2fc_shost_template.can_queue = hba->max_outstanding_cmds; > > BTW, surely we should be setting shost->can_queue = > hba->max_outstanding_cmds after scsi_host_alloc() and not modifying > bnx2fc_shost_template, right? The series is already huge, so this stuff > would be done separately, I suppose. Hi John, If anyone else wants to work on this that's fine with me. My view is that the SCSI core should support declaring host templates const but I'm not sure it's worth it to make changes in old drivers such that their SCSI host template can be declared const. One class of SCSI LLDs that does not have a const SCSI host template are the NCR drivers. The NCR SCSI host controller was popular 40 years ago. There are probably not many working SCSI devices left that are based on this SCSI controller. Bart.
On Mon, 6 Mar 2023, Bart Van Assche wrote: > On 3/6/23 10:55, John Garry wrote: > > On 06/03/2023 16:07, Bart Van Assche wrote: > >> Another example from drivers/scsi/bnx2fc/bnx2fc_fcoe.c: > >> > >> bnx2fc_shost_template.can_queue = hba->max_outstanding_cmds; > > > > BTW, surely we should be setting shost->can_queue = > > hba->max_outstanding_cmds after scsi_host_alloc() and not modifying > > bnx2fc_shost_template, right? The series is already huge, so this stuff > > would be done separately, I suppose. > > Hi John, > > If anyone else wants to work on this that's fine with me. My view is > that the SCSI core should support declaring host templates const but I'm > not sure it's worth it to make changes in old drivers such that their > SCSI host template can be declared const. Would it alter the driver .o files? If not, the changes won't require actual testing. > One class of SCSI LLDs that does not have a const SCSI host template are > the NCR drivers. The NCR SCSI host controller was popular 40 years ago. > There are probably not many working SCSI devices left that are based on > this SCSI controller. > True, the NCR 5380 controller was popular 40 years ago among early adopters like Sun and Apple. Is this relevant?
On 3/6/23 15:34, Finn Thain wrote: > Would it alter the driver .o files? If not, the changes won't require > actual testing. Hi Finn, My understanding is that declaring a static data structure const may move it to another section in the .o file but otherwise that the .o file should not be affected. I agree that retesting shouldn't be necessary for patches that only involve making data structures const. Thanks, Bart.
On 3/3/23 5:29 PM, Bart Van Assche wrote: > Make it explicit that the SCSI host template is not modified. > > Signed-off-by: Bart Van Assche <bvanassche@acm.org> > --- > drivers/scsi/BusLogic.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/drivers/scsi/BusLogic.c b/drivers/scsi/BusLogic.c > index f7b7ffda1161..72ceaf650b0d 100644 > --- a/drivers/scsi/BusLogic.c > +++ b/drivers/scsi/BusLogic.c > @@ -54,7 +54,7 @@ > #define FAILURE (-1) > #endif > > -static struct scsi_host_template blogic_template; > +static const struct scsi_host_template blogic_template; > > /* > blogic_drvr_options_count is a count of the number of BusLogic Driver > @@ -3663,7 +3663,7 @@ static int __init blogic_parseopts(char *options) > Get it all started > */ > > -static struct scsi_host_template blogic_template = { > +static const struct scsi_host_template blogic_template = { > .module = THIS_MODULE, > .proc_name = "BusLogic", > .write_info = blogic_wr Looks good to me. Acked-by: Khalid Aziz <khalid@gonehiking.org>
On Sat, Mar 4, 2023 at 8:31 AM Bart Van Assche <bvanassche@acm.org> wrote: > > Prepare for constifying most SCSI host template pointers by constifying > the SCSI host template pointer arguments and variables in the SCSI core. > > Cc: Christoph Hellwig <hch@lst.de> > Cc: Ming Lei <ming.lei@redhat.com> > Cc: Hannes Reinecke <hare@suse.de> > Cc: John Garry <john.garry@huawei.com> > Cc: Mike Christie <michael.christie@oracle.com> > Signed-off-by: Bart Van Assche <bvanassche@acm.org> Reviewed-by: Ming Lei <ming.lei@redhat.com>
Hi Bart! > Make it explicit that ATA host templates are not modified. > drivers/ata/pata_atiixp.c | 2 +- > drivers/ata/pata_atp867x.c | 2 +- > drivers/ata/pata_bk3710.c | 380 ++++++++++++++++++++++++++++++++ ^^^^^^^^^^^^^^^^^^^^^^^^^ Don't believe you meant to add this... > drivers/ata/pata_buddha.c | 2 +- > drivers/ata/pata_cmd640.c | 2 +-
On 3/6/23 18:00, Martin K. Petersen wrote: >> Make it explicit that ATA host templates are not modified. > >> drivers/ata/pata_atiixp.c | 2 +- >> drivers/ata/pata_atp867x.c | 2 +- >> drivers/ata/pata_bk3710.c | 380 ++++++++++++++++++++++++++++++++ > ^^^^^^^^^^^^^^^^^^^^^^^^^ > Don't believe you meant to add this... Hi Martin, Thanks for having reported this. The above is the result of having resolved a rebase conflict incorrectly. I will remove file drivers/ata/pata_bk3710.c from this patch. Thanks, Bart.
On 3/7/23 11:00, Martin K. Petersen wrote: > > Hi Bart! > >> Make it explicit that ATA host templates are not modified. > >> drivers/ata/pata_atiixp.c | 2 +- >> drivers/ata/pata_atp867x.c | 2 +- >> drivers/ata/pata_bk3710.c | 380 ++++++++++++++++++++++++++++++++ > ^^^^^^^^^^^^^^^^^^^^^^^^^ > Don't believe you meant to add this... Oops. Yes, indeed, good catch ! >> drivers/ata/pata_buddha.c | 2 +- >> drivers/ata/pata_cmd640.c | 2 +- >
On 09/03/2023 18:42, Bart Van Assche wrote: > On 3/6/23 05:10, John Garry wrote: >> Apart from that, I will say that I haven't studied the driver in >> detail, but my impression is that we should just set this flag >> per-shost in base_vha->host.active_mode, and not the host template >> supported_mode member. Indeed, we don't even seem to be making this >> driver scsi_host_template as const in this series, which I thought was >> the aim (and I assume because of this). > > This patch is necessary because this patch prevents to declare the > 'hostt' member of struct Scsi_Host const. ok, sure, but I am just saying that that the pre-existing code looks suspicious. However, as I mentioned before, things like this (modifying the host template) could be improved later by someone. Thanks, John