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?

Arun Easi
2022-08-18T00:45:13Z

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.
>
> >
> > 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.

Regards,
-Arun