Re: [EXT] Re: WTF: patch "[PATCH] scsi: qla2xxx: Fix response queue handler reading stale" was seriously submitted to be applied to the 5.19-stable tree?

[Greg KH <gregkh@linuxfoundation.org>]

On Mon, Aug 15, 2022 at 03:35:09PM -0700, Arun Easi wrote:
> Hi Greg,
> 
> On Sat, 13 Aug 2022, 6:46am, Greg KH wrote:
> 
> > 
> > ----------------------------------------------------------------------
> > On Sat, Aug 13, 2022 at 03:30:37PM +0200, gregkh@linuxfoundation.org wrote:
> > > The patch below was submitted to be applied to the 5.19-stable tree.
> > > 
> > > I fail to see how this patch meets the stable kernel rules as found at
> > > Documentation/process/stable-kernel-rules.rst.
> > > 
> > > I could be totally wrong, and if so, please respond to 
> > > <stable@vger.kernel.org> and let me know why this patch should be
> > > applied.  Otherwise, it is now dropped from my patch queues, never to be
> > > seen again.
> > > 
> > > thanks,
> > > 
> > > greg k-h
> > > 
> > > ------------------ original commit in Linus's tree ------------------
> > > 
> > > >From b1f707146923335849fb70237eec27d4d1ae7d62 Mon Sep 17 00:00:00 2001
> > > From: Arun Easi <aeasi@marvell.com>
> > > Date: Tue, 12 Jul 2022 22:20:39 -0700
> > > Subject: [PATCH] scsi: qla2xxx: Fix response queue handler reading stale
> > >  packets
> > > 
> > > On some platforms, the current logic of relying on finding new packet
> > > solely based on signature pattern can lead to driver reading stale
> > > packets. Though this is a bug in those platforms, reduce such exposures by
> > > limiting reading packets until the IN pointer.
> > > 
> > > Two module parameters are introduced:
> > > 
> > >   ql2xrspq_follow_inptr:
> > > 
> > >     When set, on newer adapters that has queue pointer shadowing, look for
> > >     response packets only until response queue in pointer.
> > > 
> > >     When reset, response packets are read based on a signature pattern
> > >     logic (old way).
> > > 
> > >   ql2xrspq_follow_inptr_legacy:
> > > 
> > >     Like ql2xrspq_follow_inptr, but for those adapters where there is no
> > >     queue pointer shadowing.
> > 
> > On a meta-note, this patch seems VERY wrong.  You are adding a
> > driver-wide module option for a device-specific action.  That should be
> > a device-specific functionality, not a module.
> > 
> > Again, as I say many times, this isn't the 1990's, please never add new
> > module parameters.  Use the other interfaces we have in the kernel to
> > configure individual devices properly, module parameters are not to be
> > used for that at all for anything new.
> > 
> > So, can you revert this commit and do it properly please?
> > 
> 
> The reason I chose module parameter way was because of these primarily:
> 
> 1 As this is a platform specific issue, this behavior does not require a 
>   device granular level tuning. Either all the said adapters turns the 
>   behavior on or off.

What is going to allow a user to know to do this or not?  Why is this
needed at all, and why doesn't the driver automatically know what to do
either based on the device id, or the platform, or the workload?

Forcing a user to pick something for "tuning" like this is horrible and
messy and almost impossible to support properly over time.  Just do it
in the driver automatically and then there's no need for any options at
all.

> 2 Module parameters allowed persistence without complexity: Since this 
>   adapter is also used in booting on some environments, module parameter 
>   allowed the configurability as well as simplicity.

Just because it is easy does not mean it is correct.  It is a
device-specific option being applied to ALL devices in the system, which
feels wrong.  If it is correct, then just do it automatically in the
driver based on platform information.

> If the above approach is discouraged, the alternatives that comes to my 
> mind are:
> 
> 1 Add a sysfs node 

Sure!

> 2 Add a debugfs node

Only if this really is only for debugging as that's what debugfs is for.
You can never rely on debugfs to be present or accessable for anything
that the system relies on for functionality.

> 3 /proc/sys/kernel ? (but that is not per adapter specific)

Ick, no.

> 4 Add a udev script to watch for PCI adapter addition and set/reset 1, 2 
>   or 3

Your udev script will tie into the sysfs node.

> If udev route is taken, I'd have to come up with a configuration file to 
> save tunable state, which could be a bit cumbersome and needs 
> documentation and be different (in terms of script location/submitting) 
> distro to distro.

How is a module parameter saving any state anywhere?  Your sysfs rule
should be identical to the rule that causes you to write the module
parameter file out to the device.

And then that logic, again, really should just be in the driver itself
with no option needed at all.

Again, resist the option to force a user to do anything, that's messy
and painful and not what a kernel should do if at all possible.

thanks,

greg k-h