[+cc Thomas] On Mon, Nov 11, 2024 at 11:17:38PM +0200, Leon Romanovsky wrote: > On Mon, Nov 11, 2024 at 02:41:04PM -0600, Bjorn Helgaas wrote: > > On Thu, Nov 07, 2024 at 08:56:56PM +0200, Leon Romanovsky wrote: > > > From: Leon Romanovsky <leonro@xxxxxxxxxx> > > > > > > The Vital Product Data (VPD) attribute is not readable by regular > > > user without root permissions. Such restriction is not really needed > > > for many devices in the world, as data presented in that VPD is not > > > sensitive and access to the HW is safe and tested. > > > > > > This change aligns the permissions of the VPD attribute to be accessible > > > for read by all users, while write being restricted to root only. > > > > > > For the driver, there is a need to opt-in in order to allow this > > > functionality. > > > > I don't think the use case is very strong (and not included at all > > here). > > I will add the use case, which is running monitoring application without > need to be root. IMHO reducing number of applications that require > privileged access is a very strong case. I personally try to avoid > applications with root/setuid privileges. Avoiding root/setuid is a very good thing. I just don't think using VPD directly from userspace is a great idea because VPD is so slow and sometimes unreliable to read. And apparently this is a pretty unusual situation since I haven't heard similar requests for other devices. Sort of ironic that some vendors, especially Intel and AMD, add new Device IDs for devices that work exactly the same as their predecessors, so we are continually adding to the pci_device_id tables, while here we apparently the same Device ID is used for devices that differ in ways we actually want to know about. > > If we do need to do this, I think it's a property of the device, not > > the driver. > > But how will device inform PCI core about safe VPD read? > Should I add new field to struct pci_device_id? Add a quirk? > Otherwise, I will need to add a line "pci_dev->downgrade_vpd_read=true" > to mlx5 probe function and it won't change a lot from current > implementation. To me it looks like a pci_dev bit, not a pci_driver bit. I would set it .probe() so all the code is in one place. And it's not related to a device defect, as most quirks are. Bjorn
