LKML Archive on lore.kernel.org
help / color / mirror / Atom feed
* [PATCH] PCI/sysfs: add write attribute for boot_vga
@ 2021-08-31  7:55 Zhenneng Li
  0 siblings, 0 replies; 9+ messages in thread
From: Zhenneng Li @ 2021-08-31  7:55 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Zhenneng Li

Add writing attribute for boot_vga sys node,
so we can config default video display
output dynamically when there are two video
cards on a machine.

Xorg server will determine running on which
video card based on boot_vga node's value.

Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
---
 drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 7bbf2673c7f2..a6ba19ce7adb 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
 			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
 			     IORESOURCE_ROM_SHADOW));
 }
-static DEVICE_ATTR_RO(boot_vga);
+
+static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct pci_dev *vga_dev = vga_default_device();
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val != 1)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (pdev != vga_dev)
+		vga_set_default_device(pdev);
+
+	return count;
+}
+static DEVICE_ATTR_RW(boot_vga);
 
 static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-28 23:37     ` Bjorn Helgaas
@ 2021-09-29  1:45       ` 李真能
  0 siblings, 0 replies; 9+ messages in thread
From: 李真能 @ 2021-09-29  1:45 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci, linux-kernel


在 2021/9/29 上午7:37, Bjorn Helgaas 写道:
> On Mon, Sep 27, 2021 at 11:45:59AM +0800, 李真能 wrote:
>> 在 2021/9/27 上午4:20, Bjorn Helgaas 写道:
>>> On Sun, Sep 26, 2021 at 03:15:39PM +0800, Zhenneng Li wrote:
>>>> Add writing attribute for boot_vga sys node,
>>>> so we can config default video display
>>>> output dynamically when there are two video
>>>> cards on a machine.
>>>>
>>>> Xorg server will determine running on which
>>>> video card based on boot_vga node's value.
>>> When you repost this, please take a look at the git commit log history
>>> and make yours similar.  Specifically, the subject should start with a
>>> capital letter, and the body should be rewrapped to fill 75
>>> characters.
>>>
>>> Please contrast this with the existing VGA arbiter.  See
>>> Documentation/gpu/vgaarbiter.rst.  It sounds like this may overlap
>>> with the VGA arbiter functionality, so this should explain why we need
>>> both and how they interact.
>> "Some "legacy" VGA devices implemented on PCI typically have the same
>> hard-decoded addresses as they did on ISA. When multiple PCI devices are
>> accessed at same time they need some kind of coordination. ", this is the
>> explain of config VGA_ARB, that is to say, some legacy vga devices need use
>> the same pci bus address, if user app(such as xorg) want access card A, but
>> card A and card B have same bus address,  then VGA agaarbiter will determine
>> will card to be accessed.
> Yes.  I think the arbiter also provides an interface for controlling
> the routing of these legacy resources.
>
> Your patch changes the kernel's idea of the default VGA device, but
> doesn't affect the resource routing, AFAICT.
>
>> And xorg will read boot_vga to determine which graphics card is the primary
>> graphics output device.
> Doesn't xorg also have its own mechanism for selecting which graphics
> device to use?
>
> Is the point here that you want to write the sysfs file to select the
> device instead of changing the xorg configuration?  If it's possible
> to configure xorg directly to use different devices, my inclination
> would be to use that instead of doing it via sysfs.
Thanks for reminding, Xorg has the config option "PrimaryGPU", it has 
the same func as boot_vga.
>
>> That is the difference about boot_vga and vgaarbiter.
>>
>>>> Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
>>>> ---
>>>>    drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
>>>>    1 file changed, 23 insertions(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>>>> index 7bbf2673c7f2..a6ba19ce7adb 100644
>>>> --- a/drivers/pci/pci-sysfs.c
>>>> +++ b/drivers/pci/pci-sysfs.c
>>>> @@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>>>>    			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
>>>>    			     IORESOURCE_ROM_SHADOW));
>>>>    }
>>>> -static DEVICE_ATTR_RO(boot_vga);
>>>> +
>>>> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
>>>> +			      const char *buf, size_t count)
>>>> +{
>>>> +	unsigned long val;
>>>> +	struct pci_dev *pdev = to_pci_dev(dev);
>>>> +	struct pci_dev *vga_dev = vga_default_device();
>>>> +
>>>> +	if (kstrtoul(buf, 0, &val) < 0)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (val != 1)
>>>> +		return -EINVAL;
>>>> +
>>>> +	if (!capable(CAP_SYS_ADMIN))
>>>> +		return -EPERM;
>>>> +
>>>> +	if (pdev != vga_dev)
>>>> +		vga_set_default_device(pdev);
>>>> +
>>>> +	return count;
>>>> +}
>>>> +static DEVICE_ATTR_RW(boot_vga);
>>>>    static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>>>>    			       struct bin_attribute *bin_attr, char *buf,
>>>> -- 
>>>> 2.25.1
>>>>
>>>>
>>>> No virus found
>>>> 		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-27  3:45   ` 李真能
  2021-09-27  3:57     ` Kai-Heng Feng
@ 2021-09-28 23:37     ` Bjorn Helgaas
  2021-09-29  1:45       ` 李真能
  1 sibling, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2021-09-28 23:37 UTC (permalink / raw)
  To: 李真能; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Mon, Sep 27, 2021 at 11:45:59AM +0800, 李真能 wrote:
> 
> 在 2021/9/27 上午4:20, Bjorn Helgaas 写道:
> > On Sun, Sep 26, 2021 at 03:15:39PM +0800, Zhenneng Li wrote:
> > > Add writing attribute for boot_vga sys node,
> > > so we can config default video display
> > > output dynamically when there are two video
> > > cards on a machine.
> > > 
> > > Xorg server will determine running on which
> > > video card based on boot_vga node's value.
> > When you repost this, please take a look at the git commit log history
> > and make yours similar.  Specifically, the subject should start with a
> > capital letter, and the body should be rewrapped to fill 75
> > characters.
> > 
> > Please contrast this with the existing VGA arbiter.  See
> > Documentation/gpu/vgaarbiter.rst.  It sounds like this may overlap
> > with the VGA arbiter functionality, so this should explain why we need
> > both and how they interact.
> 
> "Some "legacy" VGA devices implemented on PCI typically have the same
> hard-decoded addresses as they did on ISA. When multiple PCI devices are
> accessed at same time they need some kind of coordination. ", this is the
> explain of config VGA_ARB, that is to say, some legacy vga devices need use
> the same pci bus address, if user app(such as xorg) want access card A, but
> card A and card B have same bus address,  then VGA agaarbiter will determine
> will card to be accessed.

Yes.  I think the arbiter also provides an interface for controlling
the routing of these legacy resources.

Your patch changes the kernel's idea of the default VGA device, but
doesn't affect the resource routing, AFAICT.

> And xorg will read boot_vga to determine which graphics card is the primary
> graphics output device.

Doesn't xorg also have its own mechanism for selecting which graphics
device to use?

Is the point here that you want to write the sysfs file to select the
device instead of changing the xorg configuration?  If it's possible
to configure xorg directly to use different devices, my inclination
would be to use that instead of doing it via sysfs.

> That is the difference about boot_vga and vgaarbiter.
>
> > > Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
> > > ---
> > >   drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
> > >   1 file changed, 23 insertions(+), 1 deletion(-)
> > > 
> > > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > > index 7bbf2673c7f2..a6ba19ce7adb 100644
> > > --- a/drivers/pci/pci-sysfs.c
> > > +++ b/drivers/pci/pci-sysfs.c
> > > @@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
> > >   			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
> > >   			     IORESOURCE_ROM_SHADOW));
> > >   }
> > > -static DEVICE_ATTR_RO(boot_vga);
> > > +
> > > +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
> > > +			      const char *buf, size_t count)
> > > +{
> > > +	unsigned long val;
> > > +	struct pci_dev *pdev = to_pci_dev(dev);
> > > +	struct pci_dev *vga_dev = vga_default_device();
> > > +
> > > +	if (kstrtoul(buf, 0, &val) < 0)
> > > +		return -EINVAL;
> > > +
> > > +	if (val != 1)
> > > +		return -EINVAL;
> > > +
> > > +	if (!capable(CAP_SYS_ADMIN))
> > > +		return -EPERM;
> > > +
> > > +	if (pdev != vga_dev)
> > > +		vga_set_default_device(pdev);
> > > +
> > > +	return count;
> > > +}
> > > +static DEVICE_ATTR_RW(boot_vga);
> > >   static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> > >   			       struct bin_attribute *bin_attr, char *buf,
> > > -- 
> > > 2.25.1
> > > 
> > > 
> > > No virus found
> > > 		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-27  3:45   ` 李真能
@ 2021-09-27  3:57     ` Kai-Heng Feng
  2021-09-28 23:37     ` Bjorn Helgaas
  1 sibling, 0 replies; 9+ messages in thread
From: Kai-Heng Feng @ 2021-09-27  3:57 UTC (permalink / raw)
  To: 李真能; +Cc: Bjorn Helgaas, Bjorn Helgaas, Linux PCI, LKML

On Mon, Sep 27, 2021 at 11:46 AM 李真能 <lizhenneng@kylinos.cn> wrote:
>
>
> 在 2021/9/27 上午4:20, Bjorn Helgaas 写道:
> > On Sun, Sep 26, 2021 at 03:15:39PM +0800, Zhenneng Li wrote:
> >> Add writing attribute for boot_vga sys node,
> >> so we can config default video display
> >> output dynamically when there are two video
> >> cards on a machine.
> >>
> >> Xorg server will determine running on which
> >> video card based on boot_vga node's value.
> > When you repost this, please take a look at the git commit log history
> > and make yours similar.  Specifically, the subject should start with a
> > capital letter, and the body should be rewrapped to fill 75
> > characters.
> >
> > Please contrast this with the existing VGA arbiter.  See
> > Documentation/gpu/vgaarbiter.rst.  It sounds like this may overlap
> > with the VGA arbiter functionality, so this should explain why we need
> > both and how they interact.
>
> "Some "legacy" VGA devices implemented on PCI typically have the same
> hard-decoded addresses as they did on ISA. When multiple PCI devices are
> accessed at same time they need some kind of coordination. ", this is
> the explain of config VGA_ARB, that is to say, some legacy vga devices
> need use the same pci bus address, if user app(such as xorg) want access
> card A, but card A and card B have same bus address,  then VGA
> agaarbiter will determine will card to be accessed.
>
> And xorg will read boot_vga to determine which graphics card is the
> primary graphics output device.
>
> That is the difference about boot_vga and vgaarbiter.

So does kernel select the wrong card for boot VGA?
Or is this a feature that to switch Xorg's graphics renderer?
From what you described, it seems to be the latter one, and I think it
should be implemented in Xorg.

Kai-Heng

>
>
>
> >
> >> Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
> >> ---
> >>   drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
> >>   1 file changed, 23 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> >> index 7bbf2673c7f2..a6ba19ce7adb 100644
> >> --- a/drivers/pci/pci-sysfs.c
> >> +++ b/drivers/pci/pci-sysfs.c
> >> @@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
> >>                        !!(pdev->resource[PCI_ROM_RESOURCE].flags &
> >>                           IORESOURCE_ROM_SHADOW));
> >>   }
> >> -static DEVICE_ATTR_RO(boot_vga);
> >> +
> >> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
> >> +                          const char *buf, size_t count)
> >> +{
> >> +    unsigned long val;
> >> +    struct pci_dev *pdev = to_pci_dev(dev);
> >> +    struct pci_dev *vga_dev = vga_default_device();
> >> +
> >> +    if (kstrtoul(buf, 0, &val) < 0)
> >> +            return -EINVAL;
> >> +
> >> +    if (val != 1)
> >> +            return -EINVAL;
> >> +
> >> +    if (!capable(CAP_SYS_ADMIN))
> >> +            return -EPERM;
> >> +
> >> +    if (pdev != vga_dev)
> >> +            vga_set_default_device(pdev);
> >> +
> >> +    return count;
> >> +}
> >> +static DEVICE_ATTR_RW(boot_vga);
> >>
> >>   static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
> >>                             struct bin_attribute *bin_attr, char *buf,
> >> --
> >> 2.25.1
> >>
> >>
> >> No virus found
> >>              Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-26 20:20 ` Bjorn Helgaas
@ 2021-09-27  3:45   ` 李真能
  2021-09-27  3:57     ` Kai-Heng Feng
  2021-09-28 23:37     ` Bjorn Helgaas
  0 siblings, 2 replies; 9+ messages in thread
From: 李真能 @ 2021-09-27  3:45 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: Bjorn Helgaas, linux-pci, linux-kernel


在 2021/9/27 上午4:20, Bjorn Helgaas 写道:
> On Sun, Sep 26, 2021 at 03:15:39PM +0800, Zhenneng Li wrote:
>> Add writing attribute for boot_vga sys node,
>> so we can config default video display
>> output dynamically when there are two video
>> cards on a machine.
>>
>> Xorg server will determine running on which
>> video card based on boot_vga node's value.
> When you repost this, please take a look at the git commit log history
> and make yours similar.  Specifically, the subject should start with a
> capital letter, and the body should be rewrapped to fill 75
> characters.
>
> Please contrast this with the existing VGA arbiter.  See
> Documentation/gpu/vgaarbiter.rst.  It sounds like this may overlap
> with the VGA arbiter functionality, so this should explain why we need
> both and how they interact.

"Some "legacy" VGA devices implemented on PCI typically have the same 
hard-decoded addresses as they did on ISA. When multiple PCI devices are 
accessed at same time they need some kind of coordination. ", this is 
the explain of config VGA_ARB, that is to say, some legacy vga devices 
need use the same pci bus address, if user app(such as xorg) want access 
card A, but card A and card B have same bus address,  then VGA 
agaarbiter will determine will card to be accessed.

And xorg will read boot_vga to determine which graphics card is the 
primary graphics output device.

That is the difference about boot_vga and vgaarbiter.



>
>> Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
>> ---
>>   drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
>>   1 file changed, 23 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
>> index 7bbf2673c7f2..a6ba19ce7adb 100644
>> --- a/drivers/pci/pci-sysfs.c
>> +++ b/drivers/pci/pci-sysfs.c
>> @@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>>   			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
>>   			     IORESOURCE_ROM_SHADOW));
>>   }
>> -static DEVICE_ATTR_RO(boot_vga);
>> +
>> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
>> +			      const char *buf, size_t count)
>> +{
>> +	unsigned long val;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct pci_dev *vga_dev = vga_default_device();
>> +
>> +	if (kstrtoul(buf, 0, &val) < 0)
>> +		return -EINVAL;
>> +
>> +	if (val != 1)
>> +		return -EINVAL;
>> +
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
>> +	if (pdev != vga_dev)
>> +		vga_set_default_device(pdev);
>> +
>> +	return count;
>> +}
>> +static DEVICE_ATTR_RW(boot_vga);
>>   
>>   static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>>   			       struct bin_attribute *bin_attr, char *buf,
>> -- 
>> 2.25.1
>>
>>
>> No virus found
>> 		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-26 20:00 ` Krzysztof Wilczyński
@ 2021-09-27  3:06   ` 李真能
  0 siblings, 0 replies; 9+ messages in thread
From: 李真能 @ 2021-09-27  3:06 UTC (permalink / raw)
  To: Krzysztof Wilczyński
  Cc: Bjorn Helgaas, Huacai Chen, Kai-Heng Feng, linux-pci, linux-kernel


在 2021/9/27 上午4:00, Krzysztof Wilczyński 写道:
> [+cc Huacai and Kai-Heng as they are working in this area]
>
> Hi,
>
> Thank you for sending the patch over.
>
> I assume this is simply a resend (rather than a v2), as I see no code
> changes from the previous version you sent some time ago.
Sorry, I haven't receive any reply about this email, so I resend this.
>
>> Add writing attribute for boot_vga sys node,
>> so we can config default video display
>> output dynamically when there are two video
>> cards on a machine.
> That's OK, but why are you adding this?  What problem does it solve and
> what is the intended user here?  Might be worth adding a little bit more
> details about why this new sysfs attribute is needed.
Xorg will detemine which graphics is prime output device according 
boot_vga, if there are two graphics card, and we want xorg output 
display to diffent graphics card, we can echo 1 to boot_vga.
>
> I also need to ask, as I am not sure myself, whether this is OK to do after
> booting during runtime?  What do you think Bjorn, Huacai and Kai-Heng?

I have test this function, during runtime, if xorg's graphics output 
device is card A, then we echo 1 to boot_vga of card B, and then restart 
xorg, xorg will output to card B, if we want xorg always output to card 
B, we can echo 1 to boot_vga of card B before xorg started in daemon 
process.

> Also, please correctly capitalise the subject - have a look at previous
> commit messages to see how it should look like.
>
>> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
>> +			      const char *buf, size_t count)
>> +{
>> +	unsigned long val;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	struct pci_dev *vga_dev = vga_default_device();
>> +
>> +	if (kstrtoul(buf, 0, &val) < 0)
>> +		return -EINVAL;
>> +
>> +	if (val != 1)
>> +		return -EINVAL;
> Since this is a completely new API have a look at kstrtobool() over
> kstrtoul() as the former was created to handle user input more
> consistently.
As I want restrict available value  to 1, if we use  kstrtobool(), it 
will be available when user input other value.
>
>> +	if (!capable(CAP_SYS_ADMIN))
>> +		return -EPERM;
>> +
> Check for CAP_SYS_ADMIN is a good idea, but it has to take place before you
> attempt to accept and parse a input from the user.
>
> 	Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-26  7:15 Zhenneng Li
  2021-09-26 20:00 ` Krzysztof Wilczyński
@ 2021-09-26 20:20 ` Bjorn Helgaas
  2021-09-27  3:45   ` 李真能
  1 sibling, 1 reply; 9+ messages in thread
From: Bjorn Helgaas @ 2021-09-26 20:20 UTC (permalink / raw)
  To: Zhenneng Li; +Cc: Bjorn Helgaas, linux-pci, linux-kernel

On Sun, Sep 26, 2021 at 03:15:39PM +0800, Zhenneng Li wrote:
> Add writing attribute for boot_vga sys node,
> so we can config default video display
> output dynamically when there are two video
> cards on a machine.
> 
> Xorg server will determine running on which
> video card based on boot_vga node's value.

When you repost this, please take a look at the git commit log history
and make yours similar.  Specifically, the subject should start with a
capital letter, and the body should be rewrapped to fill 75
characters.

Please contrast this with the existing VGA arbiter.  See
Documentation/gpu/vgaarbiter.rst.  It sounds like this may overlap
with the VGA arbiter functionality, so this should explain why we need
both and how they interact.

> Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
> ---
>  drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
>  1 file changed, 23 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> index 7bbf2673c7f2..a6ba19ce7adb 100644
> --- a/drivers/pci/pci-sysfs.c
> +++ b/drivers/pci/pci-sysfs.c
> @@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
>  			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
>  			     IORESOURCE_ROM_SHADOW));
>  }
> -static DEVICE_ATTR_RO(boot_vga);
> +
> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct pci_dev *vga_dev = vga_default_device();
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	if (val != 1)
> +		return -EINVAL;
> +
> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +
> +	if (pdev != vga_dev)
> +		vga_set_default_device(pdev);
> +
> +	return count;
> +}
> +static DEVICE_ATTR_RW(boot_vga);
>  
>  static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
>  			       struct bin_attribute *bin_attr, char *buf,
> -- 
> 2.25.1
> 
> 
> No virus found
> 		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

* Re: [PATCH] PCI/sysfs: add write attribute for boot_vga
  2021-09-26  7:15 Zhenneng Li
@ 2021-09-26 20:00 ` Krzysztof Wilczyński
  2021-09-27  3:06   ` 李真能
  2021-09-26 20:20 ` Bjorn Helgaas
  1 sibling, 1 reply; 9+ messages in thread
From: Krzysztof Wilczyński @ 2021-09-26 20:00 UTC (permalink / raw)
  To: Zhenneng Li
  Cc: Bjorn Helgaas, Huacai Chen, Kai-Heng Feng, linux-pci, linux-kernel

[+cc Huacai and Kai-Heng as they are working in this area] 

Hi,

Thank you for sending the patch over.

I assume this is simply a resend (rather than a v2), as I see no code
changes from the previous version you sent some time ago.

> Add writing attribute for boot_vga sys node,
> so we can config default video display
> output dynamically when there are two video
> cards on a machine.

That's OK, but why are you adding this?  What problem does it solve and
what is the intended user here?  Might be worth adding a little bit more
details about why this new sysfs attribute is needed.

I also need to ask, as I am not sure myself, whether this is OK to do after
booting during runtime?  What do you think Bjorn, Huacai and Kai-Heng?

Also, please correctly capitalise the subject - have a look at previous
commit messages to see how it should look like.

> +static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
> +			      const char *buf, size_t count)
> +{
> +	unsigned long val;
> +	struct pci_dev *pdev = to_pci_dev(dev);
> +	struct pci_dev *vga_dev = vga_default_device();
> +
> +	if (kstrtoul(buf, 0, &val) < 0)
> +		return -EINVAL;
> +
> +	if (val != 1)
> +		return -EINVAL;

Since this is a completely new API have a look at kstrtobool() over
kstrtoul() as the former was created to handle user input more
consistently.

> +	if (!capable(CAP_SYS_ADMIN))
> +		return -EPERM;
> +

Check for CAP_SYS_ADMIN is a good idea, but it has to take place before you
attempt to accept and parse a input from the user.

	Krzysztof

^ permalink raw reply	[flat|nested] 9+ messages in thread

* [PATCH] PCI/sysfs: add write attribute for boot_vga
@ 2021-09-26  7:15 Zhenneng Li
  2021-09-26 20:00 ` Krzysztof Wilczyński
  2021-09-26 20:20 ` Bjorn Helgaas
  0 siblings, 2 replies; 9+ messages in thread
From: Zhenneng Li @ 2021-09-26  7:15 UTC (permalink / raw)
  To: Bjorn Helgaas; +Cc: linux-pci, linux-kernel, Zhenneng Li

Add writing attribute for boot_vga sys node,
so we can config default video display
output dynamically when there are two video
cards on a machine.

Xorg server will determine running on which
video card based on boot_vga node's value.

Signed-off-by: Zhenneng Li <lizhenneng@kylinos.cn>
---
 drivers/pci/pci-sysfs.c | 24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 7bbf2673c7f2..a6ba19ce7adb 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -664,7 +664,29 @@ static ssize_t boot_vga_show(struct device *dev, struct device_attribute *attr,
 			  !!(pdev->resource[PCI_ROM_RESOURCE].flags &
 			     IORESOURCE_ROM_SHADOW));
 }
-static DEVICE_ATTR_RO(boot_vga);
+
+static ssize_t boot_vga_store(struct device *dev, struct device_attribute *attr,
+			      const char *buf, size_t count)
+{
+	unsigned long val;
+	struct pci_dev *pdev = to_pci_dev(dev);
+	struct pci_dev *vga_dev = vga_default_device();
+
+	if (kstrtoul(buf, 0, &val) < 0)
+		return -EINVAL;
+
+	if (val != 1)
+		return -EINVAL;
+
+	if (!capable(CAP_SYS_ADMIN))
+		return -EPERM;
+
+	if (pdev != vga_dev)
+		vga_set_default_device(pdev);
+
+	return count;
+}
+static DEVICE_ATTR_RW(boot_vga);
 
 static ssize_t pci_read_config(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr, char *buf,
-- 
2.25.1


No virus found
		Checked by Hillstone Network AntiVirus

^ permalink raw reply	[flat|nested] 9+ messages in thread

end of thread, other threads:[~2021-09-29  1:46 UTC | newest]

Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-08-31  7:55 [PATCH] PCI/sysfs: add write attribute for boot_vga Zhenneng Li
2021-09-26  7:15 Zhenneng Li
2021-09-26 20:00 ` Krzysztof Wilczyński
2021-09-27  3:06   ` 李真能
2021-09-26 20:20 ` Bjorn Helgaas
2021-09-27  3:45   ` 李真能
2021-09-27  3:57     ` Kai-Heng Feng
2021-09-28 23:37     ` Bjorn Helgaas
2021-09-29  1:45       ` 李真能

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).