RE: [PATCH v2 net] net: dsa: microchip: provide a list of valid protocols for xmit handler

[Madhuri.Sripada]

Vlad,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@xxxxxxxxx>
> Sent: Wednesday, December 6, 2023 11:32 PM
> To: Sean Nyekjaer <sean@xxxxxxxxxx>
> Cc: Madhuri Sripada - I34878 <Madhuri.Sripada@xxxxxxxxxxxxx>; Woojung
> Huh - C21699 <Woojung.Huh@xxxxxxxxxxxxx>; UNGLinuxDriver
> <UNGLinuxDriver@xxxxxxxxxxxxx>; andrew@xxxxxxx; f.fainelli@xxxxxxxxx;
> davem@xxxxxxxxxxxxx; edumazet@xxxxxxxxxx; kuba@xxxxxxxxxx;
> pabeni@xxxxxxxxxx; Arun Ramadoss - I17769
> <Arun.Ramadoss@xxxxxxxxxxxxx>; ceggers@xxxxxxx;
> netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
> Subject: Re: [PATCH v2 net] net: dsa: microchip: provide a list of valid
> protocols for xmit handler
> 
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> On Wed, Dec 06, 2023 at 06:45:12PM +0100, Sean Nyekjaer wrote:
> > > Don't just leave it there, also explain why.
> >
> > Message to me?
> >
> > /Sean
> 
> No, to Madhuri (as the To: field suggests).
> 
> In the Linux kernel it's not a good practice to put defensive checks which don't
> have a logical justification, because other people end up not knowing why
> they're there, and when they can be removed. Checking for the tagging
> protocol gives a very clear indication and traceability of why it is being done,
> on the other hand.
> 
> If the ds->tagger_data is NULL for a tagging protocol for which it was expected
> it shouldn't be, and the DSA core still decides to call
> ds->ops->connect_tag_protocol() anyway, this is a violation of the API
> contract established with all drivers that use this mechanism. Papering over a
> bug in the DSA core results in silent failures, which means that any further
> behavior is unpredictable. So I'd very much prefer the system to fail fast in case
> of a bug in the framework, so that it can be reported and fixed. With rigorous
> testing, it will fail earlier than in the production stage.
> 
> I only said "don't leave it there, also explain why" because I really don't
> appreciate review comments spreading FUD, for which I'd have to spend 20-
> 30 minutes to explain why leaving out the NULL pointer checking is, in fact,
> safe.
> 
> Of course, I am not excluding a not-yet-found bug either, but I am strongly
> encouraging Madhuri to walk through the code path and point it to us, and
> strongly discouraging lazy review comments. It's not fair for me to reply to a 5
> word sentence with a wall of text. So I replied with a phrase of comparable
> length to the suggestion.

I am new in this community and reviews. And was reviewing from code point of view where NULL check is a primary requirement and a general practice.
I understand the justification and will make a note of it in my further reviews and my kernel development as well.
Thanks for your inputs.

-Madhuri