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
