[PATCH] PCI: sysfs: Suppress FW_BUG warning when NUMA node already matches

[PATCH] PCI: sysfs: Suppress FW_BUG warning when NUMA node already matches

[lirongqing]

From: Li RongQing <lirongqing@baidu.com>

The numa_node sysfs interface allows users to manually override a PCI
device's NUMA node assignment. Currently, every write triggers a
FW_BUG warning and taints the kernel, even when writing the same value
that is already set.

Fix by comparing the new value against the existing dev->numa_node.
Only emit the warning and update the node when they differ. Writing
the same value becomes a harmless no-op.

Signed-off-by: Li RongQing <lirongqing@baidu.com>
---
 drivers/pci/pci-sysfs.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
index 16eaaf7..9f52410 100644
--- a/drivers/pci/pci-sysfs.c
+++ b/drivers/pci/pci-sysfs.c
@@ -378,11 +378,13 @@ static ssize_t numa_node_store(struct device *dev,
 	if (node != NUMA_NO_NODE && !node_online(node))
 		return -EINVAL;
 
-	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
-	pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your vendor for updates.",
-		  node);
+	if (node != dev->numa_node) {
+		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+		pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your vendor for updates.",
+			  node);
+		dev->numa_node = node;
+	}
 
-	dev->numa_node = node;
 	return count;
 }
 
-- 
2.9.4

Re: [PATCH] PCI: sysfs: Suppress FW_BUG warning when NUMA node already matches

[Krzysztof Wilczyński]

Hello,

> The numa_node sysfs interface allows users to manually override a PCI
> device's NUMA node assignment. Currently, every write triggers a
> FW_BUG warning and taints the kernel, even when writing the same value
> that is already set.

So, this works as intended, then?

What makes multiple writes to this sysfs attribute, if you don't mind me
asking?  Do you have some tool that does this?  Some automation?

Especially, that you seem to be writing the same value over and over.

>  	if (node != NUMA_NO_NODE && !node_online(node))
>  		return -EINVAL;
>  
> -	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> -	pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your vendor for updates.",
> -		  node);
> +	if (node != dev->numa_node) {
> +		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> +		pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your vendor for updates.",
> +			  node);
> +		dev->numa_node = node;
> +	}

You could invert the check and make it an early return where you just
return count.  Would save on the new indent level.

Thank you!

	Krzysztof

答复: [????] Re: [PATCH] PCI: sysfs: Suppress FW_BUG warning when NUMA node already matches

[Li,Rongqing(ACG CCN)]

> 
> Hello,
> 
> > The numa_node sysfs interface allows users to manually override a PCI
> > device's NUMA node assignment. Currently, every write triggers a
> > FW_BUG warning and taints the kernel, even when writing the same value
> > that is already set.
> 
> So, this works as intended, then?
> 

I don't know if this is intended, but it feels unreasonable―writing the same value still triggers a FW_BUG warning.

> What makes multiple writes to this sysfs attribute, if you don't mind me
> asking?  Do you have some tool that does this?  Some automation?
> 

I have a tool that triggered this warning, and I will fix my tool.

> Especially, that you seem to be writing the same value over and over.
> 
Not , but a user is allowed to write to this file,  and subsequently will generate a massive amount of kernel logs―is this considered unreasonable? Should we add some rate limit?


> >  	if (node != NUMA_NO_NODE && !node_online(node))
> >  		return -EINVAL;
> >
> > -	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> > -	pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your
> vendor for updates.",
> > -		  node);
> > +	if (node != dev->numa_node) {
> > +		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> > +		pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact
> your vendor for updates.",
> > +			  node);
> > +		dev->numa_node = node;
> > +	}
> 
> You could invert the check and make it an early return where you just return
> count.  Would save on the new indent level.
> 

Good idle, I will fix this in v2

Thanks

[Li,Rongqing] 

> Thank you!
> 
> 	Krzysztof

Re: 答复: [????] Re: [PATCH] PCI: sysfs: Suppress FW_BUG warning when NUMA node already matches

[Krzysztof Wilczy��ski]

Hello,

> > > The numa_node sysfs interface allows users to manually override a PCI
> > > device's NUMA node assignment. Currently, every write triggers a
> > > FW_BUG warning and taints the kernel, even when writing the same value
> > > that is already set.
> > 
> > So, this works as intended, then?
> > 
> 
> I don't know if this is intended, but it feels unreasonable―writing the same value still triggers a FW_BUG warning.

The pci_alert() might be a bit much.  Especially, that users often don't
have any means to contact some vendor with a bug report or anything like
this.

Bjorn has been looking at potentially removing such warnings, if there is
nothing actionable there for the user, precisely to remove the annoyance.

The taint would remain, I suppose.

> > What makes multiple writes to this sysfs attribute, if you don't mind me
> > asking?  Do you have some tool that does this?  Some automation?
> > 
> 
> I have a tool that triggered this warning, and I will fix my tool.
> 
> > Especially, that you seem to be writing the same value over and over.
> > 
> Not , but a user is allowed to write to this file,  and subsequently will generate a massive amount of kernel logs―is this considered unreasonable? Should we add some rate limit?

Sure.  No problem with that.  I was more curious about what your use case
was, if anything.

> > >  	if (node != NUMA_NO_NODE && !node_online(node))
> > >  		return -EINVAL;
> > >
> > > -	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> > > -	pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact your
> > vendor for updates.",
> > > -		  node);
> > > +	if (node != dev->numa_node) {
> > > +		add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
> > > +		pci_alert(pdev, FW_BUG "Overriding NUMA node to %d.  Contact
> > your vendor for updates.",
> > > +			  node);
> > > +		dev->numa_node = node;
> > > +	}
> > 
> > You could invert the check and make it an early return where you just return
> > count.  Would save on the new indent level.
> > 
> 
> Good idle, I will fix this in v2
> 
> Thanks

Thank you!

	Krzysztof