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

[Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>]

On Wed, Feb 15, 2023 at 09:56:46AM +0000, Kumaravel.Thiagarajan@xxxxxxxxxxxxx wrote:
> > From: Kumaravel Thiagarajan - I21417
> > <Kumaravel.Thiagarajan@xxxxxxxxxxxxx>
> > Sent: Wednesday, February 15, 2023 3:18 PM
> > To: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>; Michael Walle 
> > <michael@xxxxxxxx>
> > Cc: Tharunkumar Pasumarthi - I67821
> > <Tharunkumar.Pasumarthi@xxxxxxxxxxxxx>; UNGLinuxDriver 
> > <UNGLinuxDriver@xxxxxxxxxxxxx>; arnd@xxxxxxxx; linux- 
> > gpio@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; 
> > srinivas.kandagatla@xxxxxxxxxx
> > Subject: RE: [PATCH v5 char-misc-next] misc: microchip: pci1xxxx: Add 
> > OTP/EEPROM driver for the pci1xxxx switch
> > 
> > >
> > > On Wed, Feb 15, 2023 at 09:20:10AM +0100, Michael Walle wrote:
> > > > Hi,
> > > >
> > > > > > > Microchip's pci1xxxx is an unmanaged PCIe3.1a switch for 
> > > > > > > consumer, industrial, and automotive applications. This 
> > > > > > > switch integrates OTP and EEPROM to enable customization of 
> > > > > > > the part in the field. This patch provides the OTP/EEPROM 
> > > > > > > driver to support the
> > > same.
> > > > > >
> > > > > > Why isn't this driver using the nvmem subsystem which is 
> > > > > > usually used for OTP and EEPROM?
> > > > > Michael, these OTP and EEPROM memories do not have any fixed 
> > > > > location registers which store values (Eg. mac address, config 
> > > > > parameters, etc) at fixed offsets.
> > > > > It stores a bunch of records, each of which has some data to be 
> > > > > written into the device's hardware registers at different locations.
> > > > > These records are directly consumed by the hardware and 
> > > > > interpreted without the involvement of the software.
> > > > > Therefore, we don't require any OTP / EEPROM register map to be 
> > > > > input to the OS / driver through device tree or board files.
> > > > > I only had to enumerate two separate block devices using the 
> > > > > driver so that the config binary files can be overlayed using 
> > > > > the dd command.
> > > > > Since this is not fitting like a conventional nvme device, I 
> > > > > didn't choose the nvme subsystem.
> > > > > Please let me know your thoughts / comments if any.
> > > >
> > > > So this is only for provisioning. i.e. during manufacturing a 
> > > > board which uses this PCI bridge? There are no kernel users, nor 
> > > > is there a common interface towards user-space. But just some 
> > > > block device (why not a char device?) exposed to userspace. I 
> > > > presume there is a companion userspace application for it? Why do 
> > > > you take the extra step and have a (random) kernel interface, you 
> > > > could also just access the PCI device directly from userspace 
> > > > within your companion application, e.g. through libpci.
> > >
> > > Yeah, why not just use userspace, I missed that, thanks!
> > Greg & Michael, I do not want to expose the entire or even partial set 
> > of device registers to the user space access directly for safety reasons.

But that's all exposed here through this block device, right?

And this is already exposed to userspace today, no need to add anything
the kernel already provides this.

> > I think hardware registers shall be accessible only through safe and 
> > robust kernel mode components and that the user space shall only be 
> > able to access the device through the kernel mode services.

Again, PCI devices are already exposed to userspace today, what am I
missing?

> > I want the user to use the hardware only in those ways designated by 
> > the driver.
> > We were using the "busybox devmem" to access the hardware registers 
> > directly and to program the EEPROM / OTP.
> > But we understood that it cannot be an end user solution in all cases 
> > and based on some of the operating environments, there can be some 
> > restrictions in opening the direct hardware access to the user space.

What restrictions are you referring to?


> > Please let me know your thoughts / comments if any.
> 
> I missed one more important point. This driver is targeted not just for the manufacturing environment.
> we want to be able to update the OTP / EEPROM when the device is in the field also.

Great, then this really should just be using the firmware download
interface if you wish to write to this hardware.  Don't expose this as a
block device, as that's not what this is.

thanks,

greg k-h