Re: [PATCH 09/15] net: hbl_en: add habanalabs Ethernet driver

[Przemek Kitszel <przemyslaw.kitszel@intel.com>]

On 6/19/24 09:16, Omer Shpigelman wrote:
> On 6/18/24 17:19, Andrew Lunn wrote:

[...]

>>>>> +module_param(poll_enable, bool, 0444);
>>>>> +MODULE_PARM_DESC(poll_enable,
>>>>> +              "Enable Rx polling rather than IRQ + NAPI (0 = no, 1 = yes, default: no)");
>>>>
>>>> Module parameters are not liked. This probably needs to go away.
>>>>
>>>
>>> I see that various vendors under net/ethernet/* use module parameters.
>>> Can't we add another one?
>>
>> Look at the history of those module parameters. Do you see many added
>> in the last year? 5 years?
>>
> 
> I didn't check that prior to my submit. Regarding this "no new module
> parameters allowed" rule, is that documented anywhere? if not, is that the
> common practice? not to try to do something that was not done recently?
> how "recently" is defined?
> I just want to clarify this because it's hard to handle these submissions
> when we write some code based on existing examples but then we are
> rejected because "we don't do that here anymore".
> I want to avoid future cases of this mismatch.
> 

best way is to read netdev ML, that way you will learn what interfaces
are frowned upon and which are outright banned, sometimes you could
judge yourself knowing which interfaces are most developed recently

in this module params example - they were introduced to allow init phase
configuration of the device, that could not be postponed, what in the
general case sounds like a workaround; hardest cases include huge swaths
of (physical continuous) memory to be allocated, but for that there are
now device tree binding solutions; more typical cases for networking are
resolved via devlink reload

devlink parms are also the thing that should be used as a default for
new parameters, the best if given parameter is not driver specific quirk

poll_enable sounds like something that should be a common param,
but you have to better describe what you mean there
(see napi_poll(), "Enable Rx polling" would mean to use that as default,
do you mean busy polling or what?)