| Message ID | 20230816013802.2985145-1-liaoyu15@huawei.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [net-next] pds_core: remove redundant pci_clear_master() | expand |
On Wed, Aug 16, 2023 at 09:38:02AM +0800, Yu Liao wrote: > pci_disable_device() involves disabling PCI bus-mastering. So remove > redundant pci_clear_master(). I would say that this commit message needs to be more descriptive and explain why pci_disable_device() will actually disable PCI in these flows. According to the doc and code: 2263 * Note we don't actually disable the device until all callers of 2264 * pci_enable_device() have called pci_disable_device(). Thanks > > Signed-off-by: Yu Liao <liaoyu15@huawei.com> > --- > drivers/net/ethernet/amd/pds_core/main.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c > index 672757932246..ffe619cff413 100644 > --- a/drivers/net/ethernet/amd/pds_core/main.c > +++ b/drivers/net/ethernet/amd/pds_core/main.c > @@ -374,7 +374,6 @@ static int pdsc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > return 0; > > err_out_clear_master: > - pci_clear_master(pdev); > pci_disable_device(pdev); > err_out_free_ida: > ida_free(&pdsc_ida, pdsc->uid); > @@ -439,7 +438,6 @@ static void pdsc_remove(struct pci_dev *pdev) > pci_release_regions(pdev); > } > > - pci_clear_master(pdev); > pci_disable_device(pdev); > > ida_free(&pdsc_ida, pdsc->uid); > -- > 2.25.1 > >
On 2023/8/16 14:38, Leon Romanovsky wrote: > On Wed, Aug 16, 2023 at 09:38:02AM +0800, Yu Liao wrote: >> pci_disable_device() involves disabling PCI bus-mastering. So remove >> redundant pci_clear_master(). > > I would say that this commit message needs to be more descriptive and > explain why pci_disable_device() will actually disable PCI in these > flows. > > According to the doc and code: > 2263 * Note we don't actually disable the device until all callers of > 2264 * pci_enable_device() have called pci_disable_device(). > > Thanks > Thank you for the review. My bad, I didn't describe it clearly in commit message. I will send the v2 version and add the following explanation: do_pci_disable_device() disable PCI bus-mastering as following: 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); } pcibios_disable_device(dev); } And pci_disable_device() sets dev->is_busmaster to 0. So for pci_dev that has called pci_enable_device(), pci_disable_device() involves disabling PCI bus-mastering. Remove redundant pci_clear_master() in the following places: - In error path 'err_out_clear_master' of pdsc_probe(), pci_enable_device() has already been called. - In pdsc_remove(), pci_enable_device() has already been called in pdsc_probe(). Best regards, Yu >> >> Signed-off-by: Yu Liao <liaoyu15@huawei.com> >> --- >> drivers/net/ethernet/amd/pds_core/main.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c >> index 672757932246..ffe619cff413 100644 >> --- a/drivers/net/ethernet/amd/pds_core/main.c >> +++ b/drivers/net/ethernet/amd/pds_core/main.c >> @@ -374,7 +374,6 @@ static int pdsc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) >> return 0; >> >> err_out_clear_master: >> - pci_clear_master(pdev); >> pci_disable_device(pdev); >> err_out_free_ida: >> ida_free(&pdsc_ida, pdsc->uid); >> @@ -439,7 +438,6 @@ static void pdsc_remove(struct pci_dev *pdev) >> pci_release_regions(pdev); >> } >> >> - pci_clear_master(pdev); >> pci_disable_device(pdev); >> >> ida_free(&pdsc_ida, pdsc->uid); >> -- >> 2.25.1 >> >>
On Wed, Aug 16, 2023 at 05:39:33PM +0800, Yu Liao wrote: > On 2023/8/16 14:38, Leon Romanovsky wrote: > > On Wed, Aug 16, 2023 at 09:38:02AM +0800, Yu Liao wrote: > >> pci_disable_device() involves disabling PCI bus-mastering. So remove > >> redundant pci_clear_master(). > > > > I would say that this commit message needs to be more descriptive and > > explain why pci_disable_device() will actually disable PCI in these > > flows. > > > > According to the doc and code: > > 2263 * Note we don't actually disable the device until all callers of > > 2264 * pci_enable_device() have called pci_disable_device(). > > > > Thanks > > > Thank you for the review. My bad, I didn't describe it clearly in commit > message. I will send the v2 version and add the following explanation: > > do_pci_disable_device() disable PCI bus-mastering as following: > 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); > } > > pcibios_disable_device(dev); > } > And pci_disable_device() sets dev->is_busmaster to 0. > > So for pci_dev that has called pci_enable_device(), pci_disable_device() > involves disabling PCI bus-mastering. Remove redundant pci_clear_master() in > the following places: > - In error path 'err_out_clear_master' of pdsc_probe(), pci_enable_device() > has already been called. > - In pdsc_remove(), pci_enable_device() has already been called in pdsc_probe(). All that you need to add is a sentence that pci_enable_device() is called only once before calling to pci_disable_device() and such pci_clear_master() is not needed. Thanks
On 8/15/2023 6:38 PM, Yu Liao wrote: > > pci_disable_device() involves disabling PCI bus-mastering. So remove > redundant pci_clear_master(). > > Signed-off-by: Yu Liao <liaoyu15@huawei.com> > --- > drivers/net/ethernet/amd/pds_core/main.c | 2 -- > 1 file changed, 2 deletions(-) > > diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c > index 672757932246..ffe619cff413 100644 > --- a/drivers/net/ethernet/amd/pds_core/main.c > +++ b/drivers/net/ethernet/amd/pds_core/main.c > @@ -374,7 +374,6 @@ static int pdsc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) > return 0; > > err_out_clear_master: > - pci_clear_master(pdev); Sure, this seems to make sense. However, if we're removing this call, then we should change the name of the goto label to something like err_out_disable_device. sln > pci_disable_device(pdev); > err_out_free_ida: > ida_free(&pdsc_ida, pdsc->uid); > @@ -439,7 +438,6 @@ static void pdsc_remove(struct pci_dev *pdev) > pci_release_regions(pdev); > } > > - pci_clear_master(pdev); > pci_disable_device(pdev); > > ida_free(&pdsc_ida, pdsc->uid); > -- > 2.25.1 > >
On 2023/8/16 18:39, Leon Romanovsky wrote: > On Wed, Aug 16, 2023 at 05:39:33PM +0800, Yu Liao wrote: >> On 2023/8/16 14:38, Leon Romanovsky wrote: >>> On Wed, Aug 16, 2023 at 09:38:02AM +0800, Yu Liao wrote: >>>> pci_disable_device() involves disabling PCI bus-mastering. So remove >>>> redundant pci_clear_master(). >>> >>> I would say that this commit message needs to be more descriptive and >>> explain why pci_disable_device() will actually disable PCI in these >>> flows. >>> >>> According to the doc and code: >>> 2263 * Note we don't actually disable the device until all callers of >>> 2264 * pci_enable_device() have called pci_disable_device(). >>> >>> Thanks >>> >> Thank you for the review. My bad, I didn't describe it clearly in commit >> message. I will send the v2 version and add the following explanation: >> >> do_pci_disable_device() disable PCI bus-mastering as following: >> 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); >> } >> >> pcibios_disable_device(dev); >> } >> And pci_disable_device() sets dev->is_busmaster to 0. >> >> So for pci_dev that has called pci_enable_device(), pci_disable_device() >> involves disabling PCI bus-mastering. Remove redundant pci_clear_master() in >> the following places: >> - In error path 'err_out_clear_master' of pdsc_probe(), pci_enable_device() >> has already been called. >> - In pdsc_remove(), pci_enable_device() has already been called in pdsc_probe(). > > All that you need to add is a sentence that pci_enable_device() is > called only once before calling to pci_disable_device() and such > pci_clear_master() is not needed. > Thank you for the review. I'll make the suggested changes and send the V2. Best regards, Yu > Thanks
On 2023/8/16 22:02, Nelson, Shannon wrote: > On 8/15/2023 6:38 PM, Yu Liao wrote: >> >> pci_disable_device() involves disabling PCI bus-mastering. So remove >> redundant pci_clear_master(). >> >> Signed-off-by: Yu Liao <liaoyu15@huawei.com> >> --- >> drivers/net/ethernet/amd/pds_core/main.c | 2 -- >> 1 file changed, 2 deletions(-) >> >> diff --git a/drivers/net/ethernet/amd/pds_core/main.c >> b/drivers/net/ethernet/amd/pds_core/main.c >> index 672757932246..ffe619cff413 100644 >> --- a/drivers/net/ethernet/amd/pds_core/main.c >> +++ b/drivers/net/ethernet/amd/pds_core/main.c >> @@ -374,7 +374,6 @@ static int pdsc_probe(struct pci_dev *pdev, const struct >> pci_device_id *ent) >> return 0; >> >> err_out_clear_master: >> - pci_clear_master(pdev); > > Sure, this seems to make sense. However, if we're removing this call, then we > should change the name of the goto label to something like > err_out_disable_device. > > sln Right, I'll make changes in v2. Best regards, Yu > >> pci_disable_device(pdev); >> err_out_free_ida: >> ida_free(&pdsc_ida, pdsc->uid); >> @@ -439,7 +438,6 @@ static void pdsc_remove(struct pci_dev *pdev) >> pci_release_regions(pdev); >> } >> >> - pci_clear_master(pdev); >> pci_disable_device(pdev); >> >> ida_free(&pdsc_ida, pdsc->uid); >> -- >> 2.25.1 >> >>
diff --git a/drivers/net/ethernet/amd/pds_core/main.c b/drivers/net/ethernet/amd/pds_core/main.c index 672757932246..ffe619cff413 100644 --- a/drivers/net/ethernet/amd/pds_core/main.c +++ b/drivers/net/ethernet/amd/pds_core/main.c @@ -374,7 +374,6 @@ static int pdsc_probe(struct pci_dev *pdev, const struct pci_device_id *ent) return 0; err_out_clear_master: - pci_clear_master(pdev); pci_disable_device(pdev); err_out_free_ida: ida_free(&pdsc_ida, pdsc->uid); @@ -439,7 +438,6 @@ static void pdsc_remove(struct pci_dev *pdev) pci_release_regions(pdev); } - pci_clear_master(pdev); pci_disable_device(pdev); ida_free(&pdsc_ida, pdsc->uid);
pci_disable_device() involves disabling PCI bus-mastering. So remove redundant pci_clear_master(). Signed-off-by: Yu Liao <liaoyu15@huawei.com> --- drivers/net/ethernet/amd/pds_core/main.c | 2 -- 1 file changed, 2 deletions(-)