Hi,

czw., 11 lut 2021 o 15:19 Andrew Lunn [off-list ref] napisał(a):

On Thu, Feb 11, 2021 at 08:22:19AM +0000, Stefan Chulski wrote:

quoted

quoted

----------------------------------------------------------------------
From: <redacted>
Date: Wed, 10 Feb 2021 11:48:17 +0200

quoted

+static int bm_underrun_protect = 1;
+
+module_param(bm_underrun_protect, int, 0444);
+MODULE_PARM_DESC(bm_underrun_protect, "Set BM underrun protect
+feature (0-1), def=1");

No new module parameters, please.

Ok, I would remove new module parameters.
By the way why new module parameters forbitten?

Historically, module parameters are a bad interface for
configuration. Vendors have stuffed all sorts of random junk into
module parameters. There is little documentation. Different drivers
can have similar looking module parameters which do different
things. Or different module parameters, which actually do the same
thing. But maybe with slightly different parameters.

We get a much better overall result if you stop and think for a
while. How can this be made a generic configuration knob which
multiple vendors could use? And then add it to ethtool. Extend the
ethtool -h text and the man page. Maybe even hack some other vendors
driver to make use of it.

Or we have also found out, that pushing back on parameters like this,
the developers goes back and looks at the code, and sometimes figures
out a way to automatically do the right thing, removing the
configuration knob, and just making it all simpler for the user to
use.

I think of 2 alternatives:
* `ethtool --set-priv-flags` - in such case there is a question if
switching this particular feature in runtime is a good idea.
* New DT/ACPI property - it is a hardware feature after all, so maybe
let the user decide whether to enable it on the platform description
level.

What do you think?

Best regards,
Marcin