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

[<Kumaravel.Thiagarajan@xxxxxxxxxxxxx>]

> -----Original Message-----
> From: Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx>
> Sent: Wednesday, February 15, 2023 5:15 PM
> To: Kumaravel Thiagarajan - I21417 <Kumaravel.Thiagarajan@xxxxxxxxxxxxx>
> 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: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?
The block device created by this driver does not expose the device registers to the user space applications.
The device hardware provides separate set of registers to read and write into the OTP memory and EEPROM.
The driver uses these hardware registers and abstracts the programming logic inside and exposes the only the memory as devices to the user space.
I don't have any user program to program the device. I use the Linux dd command only.
If I want to view the contents of the memory, I can use any hex editor tool in Linux this way.

> 
> And this is already exposed to userspace today, no need to add anything the
> kernel already provides this.
Can you explain this? Are you referring to any sysfs directories / files? What is the necessity to do this? I am trying to understand this.
If this is for any debug purpose and whether kernel does this under some conditional compilation or is that the default behavior?
Even if the user is a super user, should he be allowed to access the device hardware registers mandatorily. It should depend on the policy the system owner want to adopt. Right?

> 
> > > 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?
Are you referring to any sysfs directories / files? What is the necessity to do this? I am trying to understand this.
If this is for any debug purpose and whether kernel does this under some conditional compilation or is that the default behavior?
Even if the user is a super user, should he be allowed to access the device hardware registers mandatorily. It should depend on the policy the system owner want to adopt. Right?

> 
> > > 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?
I read some articles in internet that exporting the device memory + RAM memory to the user space presents a few security issues that need to be dealt with.
I assumed therefore, exposing the device registers to the user space may not be the default behavior in some operating environments.

> 
> 
> > > 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.
As I said earlier, this device is always programmed using the Linux dd command with a 8KB file with "oflag/iflag=direct" option to bypass buffering.
Even though there is no standard file system, I thought enumerating this as a block device would give easy access and quick programming time on ATE.

Greg & Michael,
I am sorry about the delay in response.
This entire week I am into some training sessions.
If required, I would want to discuss more to understand better.

Thank you for your valuable time and information.

Regards,
Kumar