From:   <VaibhaavRam.TL@microchip.com>
To:     <gregkh@linuxfoundation.org>
CC:     <arnd@arndb.de>, <Kumaravel.Thiagarajan@microchip.com>,
        <linux-kernel@vger.kernel.org>, <UNGLinuxDriver@microchip.com>,
        <Tharunkumar.Pasumarthi@microchip.com>
Subject: RE: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add
 OTP/EEPROM driver for the pci1xxxx switch
Thread-Topic: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add
 OTP/EEPROM driver for the pci1xxxx switch
Thread-Index: AQHZTfJPZe6eTNovTEKQ1bRHyBB32q7yT44AgAZ9oqA=
Date:   Mon, 13 Mar 2023 16:01:15 +0000
Message-ID: <DM4PR11MB62385D0541F7AF8EC7E9F6CE97B99@DM4PR11MB6238.namprd11.prod.outlook.com>
References: <20230303170426.353-1-vaibhaavram.tl@microchip.com>
 <ZAm7PDSIUN5HMxUu@kroah.com>
In-Reply-To: <ZAm7PDSIUN5HMxUu@kroah.com>
Accept-Language: en-US
Content-Language: en-US
x-ms-exchange-antispam-messagedata-chunkcount: 1
x-ms-exchange-antispam-messagedata-0: =?us-ascii?Q?1yozrZhCNh5yhVTbyuXGDRPD2KlyForHOuPU2yiYstElta5P7urJAlDeW+FK?=
 =?us-ascii?Q?4tZLZ4Mc4PjTyZReh5eZeH+FypTBmVVOrqzTn4oYlS3Y9pWpgKOPR/KLdhLZ?=
 =?us-ascii?Q?Hm7aqydNz+kVHx8/b+2VdG1x9HQenJbbwlb4rIYWwL1RDRm5t0YvNj4WpisE?=
 =?us-ascii?Q?NJjNCk0gTnqAoAuV9XPTkZwfe/z2tqCPtDYjkNYM8jN+LzxCViZINIr6RxCK?=
 =?us-ascii?Q?2QWl3TZt44eZIDyrETIDQauNjAo7evaQZAhfauf+VIHGFwT8eDQCUxfH4TYb?=
 =?us-ascii?Q?2o7irMDFq4xkQNilTdgi5P/Zvb1q1V5D9FViQnUvQPF/pfPf6Oe+0oExjuPY?=
 =?us-ascii?Q?zYGoev5ZmhF2oKePXnZ8LQgZ6aStg4t9PeAd+bpBfGhIzQQjX6O4qBmKZVQs?=
 =?us-ascii?Q?k+KDmbeXJbWm9m+ZZbDsUPuDtIzqnBBWpOOw6JCw9XiJLhgpmJALAKaiQwHK?=
 =?us-ascii?Q?Aknkqp0D6THbePUQgJjs0xlgqTOYN3U/DR/mzDkamzPnYrOqlrP6GgTvx4LZ?=
 =?us-ascii?Q?oO0z78obE7A7wNs2mvrD9d0Z3C4tAsPIiVYnxyi2SNUS6o5sDe0veL2BmnGp?=
 =?us-ascii?Q?vN+Wd9/BOSoJe1vb4S/A4dA39JLoDofolDG32WFMeUySq2Lmm/co2n8c8Ulx?=
 =?us-ascii?Q?Q3/xgXhqm50UE+XCHUzodJ+gk23gvP/EufQz9Gw8hgN/KWVEyiOqfy6HC7Pu?=
 =?us-ascii?Q?VSJ5tts2o8b5WiB+tCUHEXCu+IJgNdP+yYIfjBxSeCz3Uww0rLzKzpAj6a3o?=
 =?us-ascii?Q?NlSJcwzZ1uTWNQDMkAqIJdz3+iY6RlOIulcZjf/WwN6rBPIDLYPEIycZ4sPq?=
 =?us-ascii?Q?JGLavYPs+25G1J12Qf6NwL5W1R/Z1uwAiwPZeUuxCIV3GZF+Vf+h5Z5cWHkc?=
 =?us-ascii?Q?2cxDn6CJnfnS+Yvvvw7HPkb8s1owb6hFe3X/DY89RsdFdkvR0+QxTig82LPX?=
 =?us-ascii?Q?9OOmRwiIxjk7MugjkL5PYsGfhjDToMeeP44KGvO99HWHJZ8CZohazu4ksuuL?=
 =?us-ascii?Q?K9gbs/4HWBAEwW+67DJloQG/MN3PrI2cyIGxpsByu3RSA2EFL3ynEO6FT2Do?=
 =?us-ascii?Q?gcZzizdonpOKQgST2j8vJN9/zP+MmFsXaE0W4dBhT6fsmql1g8WoxP5Hs4UV?=
 =?us-ascii?Q?I832LJ9eAPVbfb+CrLQYuz1TyTb5fJ+Q4cdsvig72XVGpMq/7iEDlm13WQF0?=
 =?us-ascii?Q?7OfBkkyb7svp6zpEnS2ASdLnpUNNU6DQQDibDJ+8ya6zB1sx8YQPbEIFxYPI?=
 =?us-ascii?Q?bdqIYY8k2T+ABsPuj62asqqL5irmJooYNyAB0NVR/SBmt4HUX53ACukba/Vx?=
 =?us-ascii?Q?NXQvFgHdlcqtHAZ7t+XS1NGNZYc1AtUOE+OqolLnXKVvOi/x2Er4ZRM15vy4?=
 =?us-ascii?Q?g98soa6c7Nf8IQfXPIvHnNORYZlh3/e0G/wYQhufVji4YWSdz6djLCpWvxYb?=
 =?us-ascii?Q?Sgy+HohFaKMShEYp+aRZUs/gl8rFXfvYp36/ETeqXBAYhLk1IjvVy9vndtSp?=
 =?us-ascii?Q?C4xez3mWMv/JzoepA+R1GbYI0GgQ6eoZ+flu/AP5jR9Lhm8vQL1tmSM8/AM4?=
 =?us-ascii?Q?+FlWfMmhpU99ddw2a3AhPnOneI2uKvtrV5RIYoa9quV2y8HO0N2Xy7qVpgRK?=
 =?us-ascii?Q?4w=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: DM4PR11MB6238.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 49936da1-e2a4-4833-faaf-08db23dc2ce5
X-MS-Exchange-CrossTenant-originalarrivaltime: 13 Mar 2023 16:01:15.5635
 (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: 94FnX1PPb3PqqPzmSHqOij05FjGZvXL6ii9WQC0AEMb18UfFq03V1zDYKE8HVK+2Noq9Bz4Y47N95i7mlSCk6gL4qxor1TlXB5bagLz4JtQ=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO1PR11MB4771
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

> From: Greg KH <gregkh@linuxfoundation.org>
> Sent: Thursday, March 9, 2023 4:26 PM
> To: VaibhaavRam TL - I69105 <VaibhaavRam.TL@microchip.com>
> Subject: Re: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add=20
> OTP/EEPROM driver for the pci1xxxx switch
>=20
> On Fri, Mar 03, 2023 at 10:34:26PM +0530, Vaibhaav Ram T.L wrote:
> > +#define MMAP_EEPROM_OFFSET(x)                (EEPROM_REG_ADDR_BASE
> + x)
> > +#define MMAP_CFG_OFFSET(x)           (CONFIG_REG_ADDR_BASE + x)
> > +#define MMAP_OTP_OFFSET(x)              (OTP_REG_ADDR_BASE + x)
>=20
> No tabs?

Ok, I will correct this in next version of patch

>=20
> > +static int set_sys_lock(struct pci1xxxx_otp_eeprom_device *priv) {
> > +     void __iomem *sys_lock =3D priv->reg_base +
> > +                              MMAP_CFG_OFFSET(CFG_SYS_LOCK_OFFSET);
>=20
> Why not do the dev_err() call here instead of having to do it=20
> everywhere you call it and check for an error?

Ok.=20

>=20
> Also, why tell userspace about this, what can they do about it?
>=20

I will remove the debug print.

>=20
> > +static void get_pointers_from_kobj(struct kobject *kobj, struct=20
> > +device
> **p_dev,
> > +                                struct pci1xxxx_otp_eeprom_device **p_=
priv,
> > +                                void __iomem **p_rb) {
> > +     *p_dev =3D container_of(kobj, struct device, kobj);
> > +     *p_priv =3D dev_get_drvdata(*p_dev);
> > +     *p_rb =3D (*p_priv)->reg_base;
>=20
> Ick, no, sorry, just open-code this whenever you need it, as sometimes=20
> you do not need all of these, right?
>

This snippets of code is repeated multiple times. So I made into a function=
.
I am using all the arguments further in the functions.
Do you suggest modifying this?

> Also, any need to verify that p_priv is not NULL?  Can that happen=20
> when the device is removed and the file is still open?

Ok, I will include a condition to check that

> > +             if (ret < 0 || (!ret && (regval &
> EEPROM_CMD_EPC_TIMEOUT_BIT)))
> > +                     return -EBUSY;
>=20
> Shouldn't you return a short read if you do not read the full amount?
> That tells userspace they need to resubmit the rest, otherwise they=20
> have no idea how many bytes were successfully read.
>

Ok.
=20
>=20
> > +     ret =3D release_sys_lock(priv);
> > +     if (ret)
> > +             dev_err(dev, "SYS_LOCK_NOT_RELEASED\n");
>=20
> No error return value?
>=20

Ok, I will correct this in next version of patch

> > +             if (ret < 0 || (!ret && (regval &
> EEPROM_CMD_EPC_TIMEOUT_BIT)))
> > +                     return -EBUSY;
>=20
> Same as above, return a short write, otherwise userspace can never=20
> recover properly.
>=20

Ok.

> > +static const struct bin_attribute pci1xxxx_otp_attr =3D {
> > +     .attr =3D {
> > +             .name =3D OTP_NAME,
> > +             .mode =3D 0777,
> > +     },
> > +     .size =3D OTP_SIZE_BYTES,
> > +     .read =3D pci1xxxx_otp_read,
> > +     .write =3D pci1xxxx_otp_write,
>=20
> You have new sysfs binary attributes, where are they documented?
>=20

Ok, I will document in Documentation/ABI

>=20
> > +     ret =3D sysfs_create_bin_file(&aux_dev->dev.kobj,
> > + &pci1xxxx_otp_attr);
>=20
> You just raced with userspace and lost.  Please never do that, use a=20
> default group instead.
>=20
> AND you totally ignored the return value here, that's obviously wrong.

Return value is handled after next few lines. My bad, the two statements sh=
ould have been kept closer.

>=20
> > +
> > +     /* Set OTP_PWR_DN to 0 to make OTP Operational */
> > +     data =3D readl(priv->reg_base +
> MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> > +     writel(data & ~OTP_PWR_DN_BIT,
> > +            priv->reg_base + MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> > +     if (ret) {
> > +             writel(OTP_PWR_DN_BIT,
> > +                    priv->reg_base +
> MMAP_OTP_OFFSET(OTP_PWR_DN_OFFSET));
> > +             return dev_err_probe(&aux_dev->dev, ret,
> > +                                  "sysfs_create_bin_file otp failed\n"=
);
> > +     }
> > +
> > +     if (is_eeprom_responsive(priv)) {
> > +             ret =3D sysfs_create_bin_file(&aux_dev->dev.kobj,
> > +                                         &pci1xxxx_eeprom_attr);
>=20
> Again, default group will handle this automatically, you should never=20
> need to call a sysfs_*() call from a driver.  Otherwise something is usua=
lly very wrong.

Are you recommending similar to this snippet?

static struct bin_attribute *pci1xxxx_bin_attributes[] =3D {
	&pci1xxxx_otp_attr,
	&pci1xxxx_eeprom_attr
	NULL,
};

static const struct attribute_group pci1xxxx_bin_attributes_group =3D {
	.bin_attrs =3D pci1xxxx_bin_attributes,
};
..
..
auxiliary_device.device.attribute_group =3D pci1xxxx_bin_attributes_group

This creates sysfs for both EEPROM and OTP at once and handles for its remo=
val, right?
But, In this case, I have to check whether EEPROM is responsive and only th=
en create sysfs for it.

Can you please provide some guidance, on how to handle this situation witho=
ut using sysfs_*().

Thanks,
Vaibhaav Ram