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@xxxxxxxxxxxxxxxxxxx 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@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxx> > > > > 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. > > 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. > 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. > > > 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? 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. Regards, -Arun > 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. >
