On Mon, Nov 11, 2024 at 03:48:04PM -0600, Bjorn Helgaas wrote: > [+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. This is one time operation during application initialization, which is fast in our devices. It is used by the NVML https://developer.nvidia.com/management-library-nvml. > And apparently this is a pretty unusual situation since I haven't heard > similar requests for other devices. Maybe they didn't bother to ask. > > 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. I'm not Intel or AMD employee and never worked there, but from what I heard it is not technical decision but outcome of how their development process is structured. > > > > 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. The advantage of quirks is that we will be able to set proper file permissions from the beginning without need to load/bind driver. As Thomas suggested, the vpd_attr_is_visible() will be something like this, which is neat: if (pdev->downgrade_vpd_read) return 644; else return 600; Thanks > > Bjorn >
