RE: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add OTP/EEPROM driver for the pci1xxxx switch

[VaibhaavRam.TL]

> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, March 9, 2023 4:26 PM
> To: VaibhaavRam TL - I69105 <VaibhaavRam.TL@xxxxxxxxxxxxx>
> Subject: Re: [PATCH v7 char-misc-next] misc: microchip: pci1xxxx: Add 
> OTP/EEPROM driver for the pci1xxxx switch
> 
> 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)
> 
> No tabs?

Ok, I will correct this in next version of patch

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

Ok. 

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

I will remove the debug print.

> 
> > +static void get_pointers_from_kobj(struct kobject *kobj, struct 
> > +device
> **p_dev,
> > +                                struct pci1xxxx_otp_eeprom_device **p_priv,
> > +                                void __iomem **p_rb) {
> > +     *p_dev = container_of(kobj, struct device, kobj);
> > +     *p_priv = dev_get_drvdata(*p_dev);
> > +     *p_rb = (*p_priv)->reg_base;
> 
> Ick, no, sorry, just open-code this whenever you need it, as sometimes 
> 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 
> 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;
> 
> 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 
> have no idea how many bytes were successfully read.
>

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

Ok, I will correct this in next version of patch

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

Ok.

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

Ok, I will document in Documentation/ABI

> 
> > +     ret = sysfs_create_bin_file(&aux_dev->dev.kobj,
> > + &pci1xxxx_otp_attr);
> 
> You just raced with userspace and lost.  Please never do that, use a 
> default group instead.
> 
> 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 should have been kept closer.

> 
> > +
> > +     /* Set OTP_PWR_DN to 0 to make OTP Operational */
> > +     data = 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 = sysfs_create_bin_file(&aux_dev->dev.kobj,
> > +                                         &pci1xxxx_eeprom_attr);
> 
> Again, default group will handle this automatically, you should never 
> need to call a sysfs_*() call from a driver.  Otherwise something is usually very wrong.

Are you recommending similar to this snippet?

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

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

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

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

Thanks,
Vaibhaav Ram