[PATCH net-next] pds_core: remove redundant pci_clear_master()

[PATCH net-next] pds_core: remove redundant pci_clear_master()

[Yu Liao]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Leon Romanovsky]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Yu Liao]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Leon Romanovsky]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Nelson, Shannon]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Yu Liao]

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

Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

[Yu Liao]

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