Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

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

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.

[...]
> +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.

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).

Krzysztof