Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

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

Hi Minwoo,

Sorry for a very late reply!

[...]
> > You might need to improve the subject a little - it should be brief but
> > still informative.
> > 
> > > __pci_set_mater() has debug log in there so that it would be better to
> > > take this function.  So take __pci_set_master() function rather than
> > > open coding it.  This patch didn't move __pci_set_master() to above to
> > > avoid churns.
> > [...]
> > 
> > It would be __pci_set_master() in the sentence above.  Also, perhaps
> > "use" would be better than "take".  Generally, this commit message might
> > need a little improvement to be more clear why are you do doing this.
> 
> Sure, if we consolidate bus master enable clear functions to a single
> one, it would be better to debug and tracing the kernel behaviors.
> 
> Let me describe this 'why' to the description.

Sounds great!  Thank you!

[...]
> > You could use pci_clear_master(), which we export and that internally
> > calls __pci_set_master(), so there would be no need to add any forward
> > declarations or to move anything around in the file.
> 
> Moving delcaration to above might be churn, and I agree with your point.

I am sure that when it makes sense, then probably folks would not
object, especially since "churn" can be subjective.

> > Having said that, there is a difference between do_pci_disable_device()
> > and how __pci_set_master() works - the latter sets the is_busmaster flag
> > accordingly on the given device whereas the former does not.  This might
> > be of some significance - not sure if we should or should not set this,
> > since the do_pci_disable_device() does not do that (perhaps it's on
> > purpose or due to some hisoric reasons).
> 
> Thanks for pointing out this.  I think the difference about
> `is_busmaster` flag looks like it should not be cleared in case of power
> suspend case:
> 
> 	# Suspend
> 	pci_pm_default_suspend()
> 		pci_disable_enabled_device()
> 
> 	# Resume
> 	pci_pm_reenable_device()
> 		pci_set_master()  <-- This is based on (is_busmaster)
> 
> 
> Please let me know if I'm missing here, and appreciate pointing that
> out.  Maybe I can post v2 patch with add an argument of whether
> `is_busmaster` shoud be set inside of the function or not to
> __pci_set_master()?
[...]

Nothing is ever simple, isn't it? :-)

We definitely need to make sure that PM can keep relying on the
is_busmaster flag to restore bus mastering to previous state after the
device would resume after being suspended.

If we add another boolean argument, then we would need to update the
__pci_set_master() only in two other places, aside of using it in the
do_pci_disable_device() function, as per (as of 5.11.1 kernel):

  File              Line Content
  drivers/pci/pci.c 4308 __pci_set_master(dev, true);
  drivers/pci/pci.c 4319 __pci_set_master(dev, false);

This is not all that terrible, provided that we _really_ do want to
change this function signature and then add another condition inside.

What do you think?  If you still like the idea, then send second version
over with all the other proposed changes.

Krzysztof