Re: [PATCH v2] PCI: Adjust visibility of boot_display attribute instead of creation

[Mario Limonciello <superm1@kernel.org>]

On 7/21/25 4:57 AM, Jonathan Cameron wrote:
> On Sun, 20 Jul 2025 10:15:08 -0500
> Mario Limonciello <superm1@kernel.org> wrote:
> 
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> There is a desire to avoid creating new sysfs files late, so instead
>> of dynamically deciding to create the boot_display attribute, make
>> it static and use sysfs_update_group() to adjust visibility on the
>> applicable devices.
>>
>> This also fixes a compilation warning when compiled without
>> CONFIG_VIDEO on sparc.
>>
>> 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>
> 
> Leaving aside the 'where to call it' discussions, a few bits of feedback on the
> code. See inline.
> 

Thanks!

> 
>> ---
>> v2:
>>   * Change to sysfs_update_group() instead
>> ---
>>   drivers/pci/pci-sysfs.c | 59 +++++++++++++++++------------------------
>>   1 file changed, 25 insertions(+), 34 deletions(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 6b1a0ae254d3a..462a90d13eb87 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -1059,37 +1059,6 @@ void pci_remove_legacy_files(struct pci_bus *b)
>>   }
>>   #endif /* HAVE_PCI_LEGACY */
>>   
>> -/**
>> - * pci_create_boot_display_file - create a file in sysfs for @dev
>> - * @pdev: dev in question
>> - *
>> - * Creates a file `boot_display` in sysfs for the PCI device @pdev
>> - * if it is the boot display device.
>> - */
>> -static int pci_create_boot_display_file(struct pci_dev *pdev)
>> -{
>> -#ifdef CONFIG_VIDEO
>> -	if (video_is_primary_device(&pdev->dev))
>> -		return sysfs_create_file(&pdev->dev.kobj, &dev_attr_boot_display.attr);
>> -#endif
>> -	return 0;
>> -}
>> -
>> -/**
>> - * pci_remove_boot_display_file - remove the boot display file for @dev
>> - * @pdev: dev in question
>> - *
>> - * Removes the file `boot_display` in sysfs for the PCI device @pdev
>> - * if it is the boot display device.
>> - */
>> -static void pci_remove_boot_display_file(struct pci_dev *pdev)
>> -{
>> -#ifdef CONFIG_VIDEO
>> -	if (video_is_primary_device(&pdev->dev))
>> -		sysfs_remove_file(&pdev->dev.kobj, &dev_attr_boot_display.attr);
>> -#endif
>> -}
>> -
>>   #if defined(HAVE_PCI_MMAP) || defined(ARCH_GENERIC_PCI_MMAP_RESOURCE)
>>   /**
>>    * pci_mmap_resource - map a PCI resource into user memory space
>> @@ -1691,6 +1660,29 @@ static const struct attribute_group pci_dev_resource_resize_group = {
>>   	.is_visible = resource_resize_is_visible,
>>   };
>>   
>> +static struct attribute *pci_display_attrs[] = {
>> +	&dev_attr_boot_display.attr,
>> +	NULL,
> 
> Trivial but I'd drop the comma after the NULL. It's at terminating entry and
> we don't want it to be easy to add stuff after it!

Thanks!
Sounds sensible.  If I spin a v4 keeping it in pci-sysfs.c I'll drop it.

> 
>> +};
>> +
>> +static umode_t pci_boot_display_visible(struct kobject *kobj,
>> +					struct attribute *a, int n)
>> +{
>> +#ifdef CONFIG_VIDEO
>> +	struct device *dev = kobj_to_dev(kobj);
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +	if (a == &dev_attr_boot_display.attr && video_is_primary_device(&pdev->dev))
> Is video_is_primary_device() stubbed out on !CONFIG_VIDEO?
> 
> There seems to be a stub in include/asm-generic/video.h
> 
> If it always is, drop the ifdef guards whilst you are here as the compiler
> can see that and remove the code anyway.
> 

I feel like there was a reason for this from earlier in the versions of 
the series introducing boot_display, but with how things are now I think 
you're right.

I'll do some test builds without CONFIG_VIDEO set to convince myself.

> 
>> +		return a->mode;
>> +#endif
>> +	return 0;
>> +}
>> +
>> +static const struct attribute_group pci_display_attr_group = {
>> +	.attrs = pci_display_attrs,
>> +	.is_visible = pci_boot_display_visible,
>> +};
>> +
>>   int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>>   {
>>   	int retval;
>> @@ -1698,7 +1690,7 @@ int __must_check pci_create_sysfs_dev_files(struct pci_dev *pdev)
>>   	if (!sysfs_initialized)
>>   		return -EACCES;
>>   
>> -	retval = pci_create_boot_display_file(pdev);
>> +	retval = sysfs_update_group(&pdev->dev.kobj, &pci_display_attr_group);
>>   	if (retval)
>>   		return retval;
>>   
>> @@ -1716,7 +1708,6 @@ void pci_remove_sysfs_dev_files(struct pci_dev *pdev)
>>   	if (!sysfs_initialized)
>>   		return;
>>   
>> -	pci_remove_boot_display_file(pdev);
>>   	pci_remove_resource_files(pdev);
>>   }
>>   
>> @@ -1755,7 +1746,6 @@ static umode_t pci_dev_attrs_are_visible(struct kobject *kobj,
>>   
>>   	if (a == &dev_attr_boot_vga.attr && pci_is_vga(pdev))
>>   		return a->mode;
>> -
> Unrelated change.
Thanks, that snuck in when I was rearranging and I totally missed it.
I dropped it in v3.

> 
>>   	return 0;
>>   }
>>   
>> @@ -1845,6 +1835,7 @@ static const struct attribute_group pcie_dev_attr_group = {
>>   
>>   const struct attribute_group *pci_dev_attr_groups[] = {
>>   	&pci_dev_attr_group,
>> +	&pci_display_attr_group,
>>   	&pci_dev_hp_attr_group,
>>   #ifdef CONFIG_PCI_IOV
>>   	&sriov_pf_dev_attr_group,
>