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

[<Tharunkumar.Pasumarthi@xxxxxxxxxxxxx>]

> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Thursday, February 9, 2023 2:25 PM
> To: Tharunkumar Pasumarthi - I67821
> <Tharunkumar.Pasumarthi@xxxxxxxxxxxxx>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the
> content is safe
> 
> > +     } while (data & EEPROM_CMD_EPC_BUSY_BIT);
> 
> That's a very busy "sit and spin" loop here, what happens if the read of the
> bit never actually succeeds?  You just locked up the system with no way to
> interrupt it :(
> 
> Please provide some sort of timeout, or way to break out of this.
> 
> > +
> > +     if (data & EEPROM_CMD_EPC_TIMEOUT_BIT) {
> > +             dev_err(&priv->pdev->dev, "EEPROM write timed out\n");
> 
> How can the timeout bit happen if the busy bit was still set?
> 
> And what can userspace do about this if it is reported?

Hi Greg,
If EEPROM_CMD_EPC_BUSY_BIT is set for more than 30ms, it will be cleared automatically by the hardware logic and EEPROM_CMD_EPC_TIMEOUT_BIT bit will be set to indicate the timeout. User space application will inform user about timeout on EEPROM write/read when this error occurs.

> 
> > +             return -EFAULT;
> 
> This return value is ONLY for when we have memory faults from reading
> to/from userspace and the kernel.  It's not a valid return value for a device
> error, sorry.  -EIO maybe?
> 
> You return this error in a number of other places in the driver that shouldn't,
> please fix this up.

Okay.

Thanks,
Tharun Kumar P