[PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Minwoo Im]

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

Signed-off-by: Minwoo Im <minwoo.im.dev@gmail.com>
---
 drivers/pci/pci.c | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/drivers/pci/pci.c b/drivers/pci/pci.c
index b9fecc25d213..b2778f475ce3 100644
--- a/drivers/pci/pci.c
+++ b/drivers/pci/pci.c
@@ -2101,15 +2101,10 @@ void __weak pcibios_disable_device(struct pci_dev *dev) {}
  */
 void __weak pcibios_penalize_isa_irq(int irq, int active) {}
 
+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);
 }
-- 
2.17.1

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Minwoo Im]

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

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Krzysztof Wilczyński]

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

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Minwoo Im]

On 21-02-24 23:46:00, Krzysztof Wilczyński wrote:
> 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.

Yes,

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

I agree with this approach.  I can try it by adding another bool
argument to decide whether to update the is_busmaster flag or not inside
of the __pci_set_master.

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

Let me prepare the next version of this patch. Thanks!

> Krzysztof

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Bjorn Helgaas]

On Thu, Mar 04, 2021 at 01:40:13PM +0900, Minwoo Im wrote:
> On 21-02-24 23:46:00, Krzysztof Wilczyński wrote:
> > 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.
> 
> Yes,
> 
> > 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):
> 
> I agree with this approach.  I can try it by adding another bool
> argument to decide whether to update the is_busmaster flag or not inside
> of the __pci_set_master.
> 
> > 
> >   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.
> 
> Let me prepare the next version of this patch. Thanks!

Can you clarify what the purpose of this patch is?  Is it to fix a
defect, improve debug output, make the code cleaner, etc?

The commit log really just describes *what* the patch does, and I'm
looking for the *why*.

Bjorn

Re: [PATCH] PCI: Take __pci_set_master in do_pci_disable_device

[Minwoo Im]

On 21-03-04 06:11:43, Bjorn Helgaas wrote:
> On Thu, Mar 04, 2021 at 01:40:13PM +0900, Minwoo Im wrote:
> > On 21-02-24 23:46:00, Krzysztof Wilczyński wrote:
> > > 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.
> > 
> > Yes,
> > 
> > > 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):
> > 
> > I agree with this approach.  I can try it by adding another bool
> > argument to decide whether to update the is_busmaster flag or not inside
> > of the __pci_set_master.
> > 
> > > 
> > >   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.
> > 
> > Let me prepare the next version of this patch. Thanks!
> 
> Can you clarify what the purpose of this patch is?  Is it to fix a
> defect, improve debug output, make the code cleaner, etc?
>
> The commit log really just describes *what* the patch does, and I'm
> looking for the *why*.

I should have described why this patch is introdcued.
do_pci_disable_device clears the Bus Master Enable bit just like what
__pci_set_master does which is kind of duplications.  Also,
__pci_set_master has debug print log which is useful for users to figure
out whether bus master is set or cleared.  This patch makes
do_pci_disable_device call __pci_set_master to take the debug log
advantages and make code cleaner by clearing duplicated codes.

> Bjorn