Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:2 as permitted sender) client-ip=2620:137:e000::3:2;
From:   <Madhuri.Sripada@microchip.com>
To:     <olteanv@gmail.com>, <sean@geanix.com>
CC:     <Woojung.Huh@microchip.com>, <UNGLinuxDriver@microchip.com>,
        <andrew@lunn.ch>, <f.fainelli@gmail.com>, <davem@davemloft.net>,
        <edumazet@google.com>, <kuba@kernel.org>, <pabeni@redhat.com>,
        <Arun.Ramadoss@microchip.com>, <ceggers@arri.de>,
        <netdev@vger.kernel.org>, <linux-kernel@vger.kernel.org>
Subject: RE: [PATCH v2 net] net: dsa: microchip: provide a list of valid
 protocols for xmit handler
Thread-Topic: [PATCH v2 net] net: dsa: microchip: provide a list of valid
 protocols for xmit handler
Thread-Index: AQHaKBRhZLeoCpYepkqcVAzm+JG6R7CcgKEggAAEfICAAAKmAIAABI+AgAEL6iA=
Date:   Thu, 7 Dec 2023 10:04:13 +0000
Message-ID: <DM6PR11MB41245D212313D53CFB793BE8E18BA@DM6PR11MB4124.namprd11.prod.outlook.com>
References: <20231206071655.1626479-1-sean@geanix.com>
 <DM6PR11MB4124D98726836442169C2C55E184A@DM6PR11MB4124.namprd11.prod.outlook.com>
 <20231206173543.ag7xb4vhcjknyiyv@skbuf>
 <2244A511-6F6F-4711-9BE0-30786B021AE5@geanix.com>
 <20231206180131.6uy3c3nazjaok2yl@skbuf>
In-Reply-To: <20231206180131.6uy3c3nazjaok2yl@skbuf>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?vm60Cliv6R4bPMyJS10jPiJWqVyvI7O7ndq/8QtsL71FFvSrS/rVLkQkNjk0?=
 =?us-ascii?Q?/ZT+UoGUsoCs4hzTem6o51AgZpDXvo8H+a2LVdZ1jpjdgJyQ7UXnPyD87LOf?=
 =?us-ascii?Q?ytBynV2I5JkgjuCUkjrny+fyUYIrRbiR1aXjyYJbAlAiA5N68SvzCDwTsdmh?=
 =?us-ascii?Q?GBQ2nHA58W62qs5CXUuNScJwmgsN6vdTrPAkoOrdIdab8wGilVVMxRoDFEvT?=
 =?us-ascii?Q?NtJAx0xJjGAY2R4BJ/gFPDCox5VJnN1PMe0L0YGp1qaOCOwxT4OJWFhfTqga?=
 =?us-ascii?Q?Yb5auNFry/8+rPxb8a3sW1yzUlLLY/9LFNT/KS95fUysUVH4aJ3evjesrVCL?=
 =?us-ascii?Q?HURXiuV520qOCzpjBEOaT0oc+JYdW70Gsy5xGdEb/WRfNEEHTw+rWLc0PbRt?=
 =?us-ascii?Q?i+by6B0gL+tLZASLhKG9uJjPPdxJCEpxjFSWcoXrHlmD3fFVEVVU57bLuNkx?=
 =?us-ascii?Q?7HLwxWH/+jukUQbx3QcyjppiX3sbDktmU0PjPS0XKf2drRD4OHUmZwf3lPHy?=
 =?us-ascii?Q?Yzw4Zc8byD2dI/PTGsk3ieqmy8TUGBy9I62zzcX5lc74fSEq2jK0e8MIcC+q?=
 =?us-ascii?Q?fQJItAE3WENB6JmekYjnh/FOWPFsPwhOqDJua2OgRa0JQ+rjt9H/MKvJscLF?=
 =?us-ascii?Q?pKyBDILHK2Z8QABmqkSjLKGDBnCb6UIe3cU8odeCOHS0D4KGOZnV6KDxM1gb?=
 =?us-ascii?Q?xkpEx7TQW5QpEqxAhHsySpTXXNjudIvDlcrM+d/yuLjh/1Gl1kU2nk91fUdX?=
 =?us-ascii?Q?EC52Ykiol0TCnGlbJd8E1NFR83OoLIf/GsBy26eMOtl8GXqi3I9RHkX3+rIg?=
 =?us-ascii?Q?P9CDvHhRhl/Mq/AT9VGCVulDH1IYNpd/JSUXYGe29QvKD50NaxXIIEn9a7ts?=
 =?us-ascii?Q?LdAwtKr/jflJGnFboAdUBUTKqDzpgwj7H4KS0WzzWap1450gOBP7kKb4q8DN?=
 =?us-ascii?Q?A9il7NZBOVxFGG6e2b4Up0eC9OLiX95/NPx6wPYs2s8spG2As3bH0HfDy4jy?=
 =?us-ascii?Q?Jfbf22BuTe2aJF5Ia65JOoBE1tSDiHn/w2vq6JAleYmR5lHAuditcctyoodr?=
 =?us-ascii?Q?JRuYHmUBDZTXpbdDuq3vuWlTySyHlUU/V92blvt20xn4JRis/jqrkqznccgl?=
 =?us-ascii?Q?0Fabj/5EXwagkBAoSpsqgvNaOl6V718DllZhTEB2M0jEF5jVftAAtvOFcRhL?=
 =?us-ascii?Q?b0lz4GvTJXBWQbXvcaUVO2dfLFKeINdATxPSysqmqBxyRSbE469OWQYQk/MC?=
 =?us-ascii?Q?ZTK+//JKhnHXNWsx72lBlOi1sdq4ONLjd5h9HXO3ObWEosOcJN0tmc2F/by5?=
 =?us-ascii?Q?iyvSqq24bDu20R4C8FFqNoJDtE7wbBSqBixkSYOHH6WbssjVnuny2rYsISnk?=
 =?us-ascii?Q?QJYf3bWX+YgWW+A9eJucpow2xSR03lb/6/NgOD3e4AhV1n2z48VMTqL+5x8A?=
 =?us-ascii?Q?dMqbffHB0z3pTQTyR3GlDD044rXXcDan2Ejdltq2lMKjqKN+Ad/Z6647tERD?=
 =?us-ascii?Q?ddghkgKEN4THSLrB8xZvv+XxovRYzT8E2OX3qmOSZBpR2r6zxMW31FD9Yt9X?=
 =?us-ascii?Q?e6h9uc775/UDJL2BOLFtFjOBamENxjIcJnwNm2lmTjUQZevPtJtZ0gFbPv8A?=
 =?us-ascii?Q?AQ=3D=3D?=
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-AuthSource: DM6PR11MB4124.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 46fa4dd5-a18c-4919-7b8e-08dbf70bdd54
X-MS-Exchange-CrossTenant-originalarrivaltime: 07 Dec 2023 10:04:13.2651
 (UTC)
X-MS-Exchange-CrossTenant-fromentityheader: Hosted
X-MS-Exchange-CrossTenant-id: 3f4057f3-b418-4d4e-ba84-d55b4e897d88
X-MS-Exchange-CrossTenant-mailboxtype: HOSTED
X-MS-Exchange-CrossTenant-userprincipalname: 3G6qdv82Z/XDIUm6lstbd1T8BOgUwxln5r41mOtUfo+vc9imEx1koqXuZX2kPQi+qp2i2tbU/52JccOzN16GqsdDs+Zpb6AZTmgIFIAmrVI=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR11MB7677
X-Spam-Status: No, score=-0.9 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED,
	DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,
	MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE
	autolearn=unavailable autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on agentk.vger.email
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (agentk.vger.email [0.0.0.0]); Thu, 07 Dec 2023 02:05:01 -0800 (PST)

Vlad,

> -----Original Message-----
> From: Vladimir Oltean <olteanv@gmail.com>
> Sent: Wednesday, December 6, 2023 11:32 PM
> To: Sean Nyekjaer <sean@geanix.com>
> Cc: Madhuri Sripada - I34878 <Madhuri.Sripada@microchip.com>; Woojung
> Huh - C21699 <Woojung.Huh@microchip.com>; UNGLinuxDriver
> <UNGLinuxDriver@microchip.com>; andrew@lunn.ch; f.fainelli@gmail.com;
> davem@davemloft.net; edumazet@google.com; kuba@kernel.org;
> pabeni@redhat.com; Arun Ramadoss - I17769
> <Arun.Ramadoss@microchip.com>; ceggers@arri.de;
> netdev@vger.kernel.org; linux-kernel@vger.kernel.org
> Subject: Re: [PATCH v2 net] net: dsa: microchip: provide a list of valid
> protocols for xmit handler
>=20
> EXTERNAL EMAIL: Do not click links or open attachments unless you know th=
e
> content is safe
>=20
> 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
>=20
> No, to Madhuri (as the To: field suggests).
>=20
> In the Linux kernel it's not a good practice to put defensive checks whic=
h 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 bein=
g done,
> on the other hand.
>=20
> If the ds->tagger_data is NULL for a tagging protocol for which it was ex=
pected
> 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 o=
ver a
> bug in the DSA core results in silent failures, which means that any furt=
her
> behavior is unpredictable. So I'd very much prefer the system to fail fas=
t in case
> of a bug in the framework, so that it can be reported and fixed. With rig=
orous
> testing, it will fail earlier than in the production stage.
>=20
> 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 fa=
ct,
> safe.
>=20
> Of course, I am not excluding a not-yet-found bug either, but I am strong=
ly
> 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 comparab=
le
> length to the suggestion.

I am new in this community and reviews. And was reviewing from code point o=
f 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 rev=
iews and my kernel development as well.
Thanks for your inputs.

-Madhuri