Re: [PATCH] PCI: Fix warning without CONFIG_VIDEO

[Mario Limonciello <superm1@kernel.org>]

On 7/18/25 9:47 PM, Manivannan Sadhasivam wrote:
> On Fri, Jul 18, 2025 at 12:26:49PM GMT, Mario Limonciello wrote:
>> On 7/18/2025 12:23 PM, Manivannan Sadhasivam wrote:
>>> On Fri, Jul 18, 2025 at 12:06:22PM GMT, Mario Limonciello wrote:
>>>> On 7/18/2025 12:00 PM, Manivannan Sadhasivam wrote:
>>>>> On Fri, Jul 18, 2025 at 08:41:33AM GMT, Mario Limonciello wrote:
>>>>>> From: Mario Limonciello <mario.limonciello@amd.com>
>>>>>>
>>>>>> When compiled without CONFIG_VIDEO pci_create_boot_display_file() will
>>>>>> never create a sysfs file for boot_display. Guard the sysfs file
>>>>>> declaration against CONFIG_VIDEO.
>>>>>>
>>>>>> Reported-by: Stephen Rothwell <sfr@canb.auug.org.au>
>>>>>> Closes: https://lore.kernel.org/linux-next/20250718224118.5b3f22b0@canb.auug.org.au/
>>>>>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>>>>>> ---
>>>>>>     drivers/pci/pci-sysfs.c | 2 ++
>>>>>>     1 file changed, 2 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>>>> index 6b1a0ae254d3a..f6540a72204d3 100644
>>>>>> --- a/drivers/pci/pci-sysfs.c
>>>>>> +++ b/drivers/pci/pci-sysfs.c
>>>>>> @@ -680,12 +680,14 @@ const struct attribute_group *pcibus_groups[] = {
>>>>>>     	NULL,
>>>>>>     };
>>>>>> +#ifdef CONFIG_VIDEO
>>>>>>     static ssize_t boot_display_show(struct device *dev, struct device_attribute *attr,
>>>>>>     				 char *buf)
>>>>>>     {
>>>>>>     	return sysfs_emit(buf, "1\n");
>>>>>>     }
>>>>>>     static DEVICE_ATTR_RO(boot_display);
>>>>>
>>>>> I failed to give my comment during the offending series itself, but it is never
>>>>> late than never. Why are we adding non-PCI attributes under bus/pci in the first
>>>>> place? Though the underlying device uses PCI as a transport, only the PCI bus
>>>>> specific attrbutes should be placed under bus/pci and the driver/peripheral
>>>>> specific attrbutes should belong to the respective bus/class/device hierarchy.
>>>>>
>>>>> Now, if other peripherals (like netdev) start adding these device specific
>>>>> attributes under bus/pci, it will turn out to be a mess.
>>>>>
>>>>> - Mani
>>>>>
>>>>
>>>> It was mostly to mirror the location of where boot_vga is, which arguably
>>>> has the same issue you raise.
>>>>
>>>
>>> Yes, I agree. But 'boot_vga' has set a bad precedence IMO.
>>>
>>>> I would be incredibly surprised if there was a proposal to add a
>>>> 'boot_display' attribute from netdev..
>>>
>>> Not 'boot_display' but why not 'boot_network' or something else. I was just
>>> merely pointing out the fact that the other subsystems can start dumping
>>> device/usecase specific attributes under bus/pci.
>>>
>>> - Mani
>>>
>>
>> This is a pretty general problem that exists that attributes are first come
>> first served.  For example amdgpu adds mem_busy_percent and it has certain
>> semantics.  Now PCI core can't add that.
>>
>> And if nouveau.ko wants to add the same thing they need to follow the same
>> semantics because userspace will look for those.
> 
> But here, userspace is not yet looking for 'boot_display' isn't it? Why do you
> need to mirror 'boot_vga' attribute?
> 

I have a pull request opened in libpciaccess; but correct it has not 
been merged.

/If/ we're to change this, what would you propose the path to be for 
this sysfs file?