Re: [EXT] Re: [PATCH v12 net-next 12/15] net: mvpp2: add BM protection underrun feature support

From: Marcin Wojtas <mw@semihalf.com>
To: Andrew Lunn <andrew@lunn.ch>
Cc: Stefan Chulski <stefanc@marvell.com>,
	David Miller <davem@davemloft.net>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
	"thomas.petazzoni@bootlin.com" <thomas.petazzoni@bootlin.com>,
	Nadav Haklai <nadavh@marvell.com>,
	Yan Markman <ymarkman@marvell.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
	"kuba@kernel.org" <kuba@kernel.org>,
	"linux@armlinux.org.uk" <linux@armlinux.org.uk>,
	"rmk+kernel@armlinux.org.uk" <rmk+kernel@armlinux.org.uk>,
	"atenart@kernel.org" <atenart@kernel.org>,
	"devicetree@vger.kernel.org" <devicetree@vger.kernel.org>,
	"robh+dt@kernel.org" <robh+dt@kernel.org>,
	"sebastian.hesselbarth@gmail.com"
	<sebastian.hesselbarth@gmail.com>,
	"gregory.clement@bootlin.com" <gregory.clement@bootlin.com>,
	"linux-arm-kernel@lists.infradead.org" 
	<linux-arm-kernel@lists.infradead.org>
Subject: Re: [EXT] Re: [PATCH v12 net-next 12/15] net: mvpp2: add BM protection underrun feature support
Date: Thu, 11 Feb 2021 20:13:24 +0100	[thread overview]
Message-ID: <CAPv3WKd48fiZmdnP+NN_FRCT1h6xmu9zO4BWAz_pgTXW2fQt9w@mail.gmail.com> (raw)
In-Reply-To: <YCU864+AH6UioNwQ@lunn.ch>

Hi,

czw., 11 lut 2021 o 15:19 Andrew Lunn <andrew@lunn.ch> napisał(a):
>
> On Thu, Feb 11, 2021 at 08:22:19AM +0000, Stefan Chulski wrote:
> >
> > >
> > > ----------------------------------------------------------------------
> > > From: <stefanc@marvell.com>
> > > Date: Wed, 10 Feb 2021 11:48:17 +0200
> > >
> > > >
> > > > +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