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 Wed, Aug 17, 2022 at 05:44:48PM -0700, Arun Easi wrote:
> On Wed, 17 Aug 2022, 5:35pm, Arun Easi wrote:
> 
> > On Tue, 16 Aug 2022, 11:20pm, Greg KH wrote:
> > 
> > > On Tue, Aug 16, 2022 at 11:17:39AM -0700, Arun Easi wrote:
> > > > On Tue, 16 Aug 2022, 2:30am, Greg KH wrote:
> > > > 
> > > > > 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?
> > > > 
> > > > The change is to err on the side of caution and make the logic 
> > > > a bit conservative at the same time providing an option for those 
> > > > platforms or architectures where the issue is not applicable, but the 
> > > > logic is causing a reduction in performance.
> > > 
> > > So this is a "enable the driver to work in a broken way" option?
> > > 
> > > > > Forcing a user to pick something for "tuning" like this is horrible and
> > > > > messy and almost impossible to support properly over time.
> > > > 
> > > > The option is intended for slightly advanced users, platform or os 
> > > > vendors etc. When it comes to an end user, I agree it is challenging to 
> > > > know if a change from default is needed or not.
> > > 
> > > That's not ok, as a driver writer you need to make it "always work",
> > > don't force the user to choose "safe vs. unsafe" options, that's passing
> > > the blame.
> > > 
> > > > > Just do it in the driver automatically and then there's no need for any 
> > > > > options at all.
> > > > 
> > > > The platform bug exhibited as driver accessing stale memory, so it is 
> > > > tough to automatically tune the value automatically.
> > > 
> > > That sounds like a real bug that you need to fix.
> > 
> > Actually, this is a platform bug that got exposed with the driver 
> > behavior.

What are you meaning by "platform"?  The kernel?  Or your hardware
device?

> > > Please revert this change and just fix the issue to always work 
> > > properly.  To have an option that allows a driver to work in a broken 
> > > way is not acceptable.
> > > 
> > > > > > 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?
> > > > 
> > > > Since module parameter could be configured via modprobe.conf/equivalent, a 
> > > > user could set the value of his choice in that file and have the value 
> > > > persisted.
> > > 
> > > Same for a udev rule, so this is not an issue.
> > > 
> > > Do you want me to send a patch that reverts this, or will you?
> > > 
> > 
> > Ok, hint taken, will send a revert followed by the patch with modparam 
> > removed.
> > 
> 
> Greg, would you be alright if the module parameter be converted into a 
> #define? That way, an advanced user, who knows what he is doing, still has 
> the option, and the chances of a regular user accidentally turning on the 
> module parameter incorrectly on the buggy platform avoided as well.

No one will ever turn on such a thing if it is a #define, as no
"enterprise" customer will ever build a custom kernel (it voids their
warranty) and for the rest of the world that uses stock kernel.org
kernels, they would never want to change the code to "enable an option
that is known to break things."

Just fix the driver to work properly, don't try to paper over it with
"change this line of code to go faster but it might break!" options.

thanks,

greg k-h