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

["Krzysztof Wilczy��ski" <kw@linux.com>]

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