Re: [PATCH] PCI: Fix warning without CONFIG_VIDEO

[Mario Limonciello <superm1@kernel.org>]

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.