netdev - Re: [PATCH net-next] pds_core: remove redundant pci_clear

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <c232243d-0c5a-c253-5e3b-81be2479b776@huawei.com>
Date: Wed, 16 Aug 2023 17:39:33 +0800
From: Yu Liao <liaoyu15@...wei.com>
To: Leon Romanovsky <leon@...nel.org>
CC: <netdev@...r.kernel.org>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <liwei391@...wei.com>, Xiongfeng Wang
	<wangxiongfeng2@...wei.com>
Subject: Re: [PATCH net-next] pds_core: remove redundant pci_clear_master()

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