Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Minwoo Im <minwoo.im.dev@gmail.com>]

On 21-02-14 19:12:16, Krzysztof Wilczyński wrote:
> Hi Minwoo,
> 
> Thank you for sending the patch over!
> 
> 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() int he 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.

> 
> [...]
> > +static void __pci_set_master(struct pci_dev *dev, bool enable);
> >  static void do_pci_disable_device(struct pci_dev *dev)
> >  {
> > -	u16 pci_command;
> > -
> > -	pci_read_config_word(dev, PCI_COMMAND, &pci_command);
> > -	if (pci_command & PCI_COMMAND_MASTER) {
> > -		pci_command &= ~PCI_COMMAND_MASTER;
> > -		pci_write_config_word(dev, PCI_COMMAND, pci_command);
> > -	}
> > +	__pci_set_master(dev, false);
> >  
> >  	pcibios_disable_device(dev);
> >  }
> 
> 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.

> 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()?  pci_clear_master() has already been exported so
that adding an argument here might be a churn :)

Thanks!

> Krzysztof