On Tue, Feb 25, 2025 at 11:47:42AM +0100, Nicolas Bouchinet wrote:
>
> On 2/25/25 02:20, Martin K. Petersen wrote:
> > Hi Nicolas!
> >
> > > --- a/drivers/scsi/scsi_sysctl.c
> > > +++ b/drivers/scsi/scsi_sysctl.c
> > > @@ -17,7 +17,9 @@ static const struct ctl_table scsi_table[] = {
> > > .data = &scsi_logging_level,
> > > .maxlen = sizeof(scsi_logging_level),
> > > .mode = 0644,
> > > - .proc_handler = proc_dointvec },
> > > + .proc_handler = proc_dointvec_minmax,
> > > + .extra1 = SYSCTL_ZERO,
> > > + .extra2 = SYSCTL_INT_MAX },
> > scsi_logging_level is a bitmask and should be unsigned.
> >
>
> Hi Martin,
>
> Thank's for your review.
>
> Does `scsi_logging_level` needs the full range of a unsigned 32-bit integer
> ?
> As it was using `proc_dointvec`, it was capped to an INT_MAX.
>
> If it effectively need the full range of an unsigned 32-bit integer, the
> `proc_handler` could be changed to `proc_douintvec` as suggested by Chuck.
And as mentioned in another patch in this series:
1. Having the upper bound be SYSCTL_INT_MAX is not necessary (as it is
silently capped by proc_dointvec_minmax, but it is good to have as it
adds to the understanding on what the range of the values are.
2. Having the lower bound capped by SYSCTL_ZERO is needed as it will
prevent ppl trying to assigning negative values to the unsigned integer
Let me know if you take this through the scsi subsystem so I know to
drop it from sysctl
Best
Reviewed-by: Joel Granados <joel.granados@xxxxxxxxxx>
>
> Best regards,
>
> Nicolas
>
--
Joel Granados
