| Message ID | 20210214110637.24750-1-minwoo.im.dev@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | PCI: Take __pci_set_master in do_pci_disable_device | expand |
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
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
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
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
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
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
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); }
__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(-)