Message-ID: <6e98eced-9882-2397-663e-94470c88ec48@intel.com>
Date:   Fri, 17 Dec 2021 13:38:06 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:91.0) Gecko/20100101
 Firefox/91.0 Thunderbird/91.4.0
Subject: Re: [PATCH 1/2] PCI: Add support for VF Resizable Bar extended cap
Content-Language: en-US
To:     =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>
CC:     <linux-pci@vger.kernel.org>, <linux-kernel@vger.kernel.org>,
        Bjorn Helgaas <bhelgaas@google.com>,
        =?UTF-8?Q?Christian_K=c3=b6nig?= <christian.koenig@amd.com>,
        Ard Biesheuvel <ardb@kernel.org>,
        "Michael J . Ruhl" <michael.j.ruhl@intel.com>,
        Rodrigo Vivi <rodrigo.vivi@intel.com>
References: <20211215141626.3090807-1-michal.winiarski@intel.com>
 <20211215141626.3090807-2-michal.winiarski@intel.com>
 <YbqGplTKl5i/1/kY@rocinante>
From:   =?UTF-8?Q?Micha=c5=82_Winiarski?= <michal.winiarski@intel.com>
In-Reply-To: <YbqGplTKl5i/1/kY@rocinante>
Content-Type: text/plain; charset="UTF-8"; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?Qk5OaGRDSDFwNjl0OHRjak8zRFVoUHZ3RlR0NE05a2gwbk5TQ2VJM2V0aC83?=
 =?utf-8?B?QTdjbFMvdVVHbjFUQXl2VFVwcUgwdXYydE8wYTR0aStXY0s0bGk1U05NZXZE?=
 =?utf-8?B?Wm9qbEIzVVlVaTNIc3E2RzBUTStHYmU1R2ZtVXhpZ21KcytMejZkU0p5dFA5?=
 =?utf-8?B?aW5Yb3ZWUE5Qeis2QU9GNC9mRTF0NTFRQnJKUm1DZUt6bCtTb2M2cVJxeEFo?=
 =?utf-8?B?N3pxajYwV0xZWS9BN3BwSzZtSDZmTEZDdEhwckJEenY2c00rMWZ6WmVaRHE1?=
 =?utf-8?B?T3d3ZE1RTjBja21yTm9KdXdEZVkyRmkyL2p2TDhqUnYzYXBwME5pTzdIN0dw?=
 =?utf-8?B?aitydUJvTUx1b2MvMXZBbFJQWTNIOStJU0h0RHgvWGlBaEdXWXo3UkV3Mk5t?=
 =?utf-8?B?cjVUdFl2Z2xXazZINjFuS3YvUlUzcFFGNi9JWnp6dWcwT0VZa0N4N2hrRFNi?=
 =?utf-8?B?M1FnS214czNoT0N4Q3dGSmRrbFpNMGVWREc3MlRCb2RlMGg5SDMweXJOSkFG?=
 =?utf-8?B?Qmdvc0I5OTh1TVZmUHl0bkQrMEZqT2lFMkdia2ZiemNjMHhNbjFFUUV3UUE2?=
 =?utf-8?B?Z2pWNUZ3SnNpYjZjbk84T0I5eWtsWDVjUFIySlpHVEZMUFdpU1FIcUgwMFZ2?=
 =?utf-8?B?QkxPN2V1cnZDRDBLbXhReExyaXJvYWhnSU9hQ2pLKzlqa29jd3dPQmI3bEJj?=
 =?utf-8?B?ZnJTYVZoZk94WUdyYlFEZGhoNmJTUm4vU3BjYTNVOXlIM2dRZVJ3WXQzL1pC?=
 =?utf-8?B?bHpKY0xmeUtjc2lSNUFxMlBrcU9CQ09ndUd4ODh4WlA1aTRENU9vWTRJRFh5?=
 =?utf-8?B?QWtDbGwwT1BwMkY5V2RlYUFWd1JwdTd6UUViVCtydlBiV3E0ZEp3cGZmRFEr?=
 =?utf-8?B?eXdVUGVyZmdZM0c1WStEY3phTFF2RlArZ1A2akpJSjhWQlk1RmJ2THRVbTI2?=
 =?utf-8?B?R1FWUkJKZjFxZXNENDNMVmlMZjRQU0FRWHNaL3BVVjFuSnFWcjV1WlN3ZHJE?=
 =?utf-8?B?aUtjUU93bDZkVHJSRm1DV1JJMkNyTkJzOHZRcTFTazIybnVrSDk5Mk1selZo?=
 =?utf-8?B?bDlKQkVUSngvSEVISjZMaElCSHVzL2VwaWF0KzBEMDdvcFNtM0hZS3puZ3BW?=
 =?utf-8?B?Y1h1eElWQ2cwMHI3M3ZyVWsyWTUzd09EL3JSSHJMZGxDSEpQUkhQWGNibkpQ?=
 =?utf-8?B?QkZWcDN2MzhnZ25tSUNnR0JsNmJtV1dJTkdvUTdTMGxIVGdqS2FNcjFmamZS?=
 =?utf-8?B?eWtTb1kxRDVOOEo2NjgzWEhiOTNUd2E2RWZUS0dWNkgxWjUyVnZYSHlna0pN?=
 =?utf-8?B?dEVqSlA3d3ZTQ0p2V0d3ZlZQMGJraUk3VVNrU3l0ZmxIenFiSHRDQ09aOXJX?=
 =?utf-8?B?V2hxdFJFcGY0cWxCc01ickI1M2hxcVEyUmF3eGQ2OGppNStueXBnQ0JjeDJa?=
 =?utf-8?B?L0RaQ2M5aGw1OXZiTExjcmprUnE1K2RyUWFQQ1NSZXdEc3k0bVA3aW5hWkhY?=
 =?utf-8?B?NitBS2IrenV6RnZuQXdpTDZwTi9STmNNSzJRcHg0a2VRa3R1VkxEMlUyR0sz?=
 =?utf-8?B?NzNvNHczcjk2aVNvcnNZdnpDYTE0bjBvSSt2amJ2Z2tITnpUOXI3ZEZuMy9a?=
 =?utf-8?B?RDJyWGw5QnJ0SVdjVXY5aEN2UUgvcCsyRDB2NGszUVpQeFRjSDUwTllweFdm?=
 =?utf-8?B?b1RBMVhlQWxTTVdJa2JNUzNzdDFoSjkyYVRJakNRLzJzV3JCcjM3MWt2VTBo?=
 =?utf-8?B?b0YzZjlTR0VoUmtweTJiQy9rRnE5OWJpcmEzMWg3MGllcjVURGs3OFl0TThl?=
 =?utf-8?B?QlltV1d0WG1yeCtkcTZidkxsQjBVRUZDemZ2Qmltb2pJaE1GNWd1dTJvbEx2?=
 =?utf-8?B?d1krM0txZWRJVDNBVDNxYW5hc0lCL0lmVDNva3RVL3dpdnRUakhncDNkazA2?=
 =?utf-8?B?M0paaHdndVU3Q1l5cU9mY3JkR2VKWHF5SkxRKzVIMmVaaFk0aEw2SmxGZXFt?=
 =?utf-8?B?d3hUZjlYeklnUWxzM0JhdmwvUEloQUUyaGpEc2pxR09vaWlWdW9RQWZxOHBq?=
 =?utf-8?B?TEJxcytxdjEwR2VyWGdWcUlxL3J2NE9tVURkVHBJaCtYNndXaFNMcWF0ZGIw?=
 =?utf-8?B?Wm9IdjZUVGo3emVDbEgxM0JTZkJ2ajc4UlhiZ01TU2pLN2xSN2tZRjN4Q2tG?=
 =?utf-8?B?ekRtMmJYK2lGMko3MnFtYlZzREhMQkhJWEZ0MjJXK2xiU0NDYkxUdEtXdzBi?=
 =?utf-8?B?NnlmTWdKNENwZk5vZnVoU0xFYTBTNE1iWWFleklrSVU1UVlOL01mOWpWKzht?=
 =?utf-8?Q?Ryy0FHqPCkNiFlF2AT?=
X-MS-Exchange-CrossTenant-Network-Message-Id: fcc2b172-5b25-4cfd-9147-08d9c15a1686
X-MS-Exchange-CrossTenant-AuthSource: DM4PR11MB5373.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 17 Dec 2021 12:38:12.1426
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 46c98d88-e344-4ed4-8496-4ed7712e255d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: m6VmgnX7MHe77QASMf22bQjzHaCgJNGrMLr1NfOqCe1RapOq/QmeMw/66L8DjKRQYqPCHYcMW1LokJ6iSU/pNvu0w1tG1GIjftXldQ49FpA=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: DM5PR11MB1292
X-OriginatorOrg: intel.com
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 16.12.2021 01:21, Krzysztof Wilczyński wrote:
> Hi Michał,
> 
> [...]
>> +static int pci_memory_decoding(struct pci_dev *dev)
>> +{
>> +	u16 cmd;
>> +
>> +	pci_read_config_word(dev, PCI_COMMAND, &cmd);
>> +	if (cmd & PCI_COMMAND_MEMORY)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +
>> +#ifdef CONFIG_PCI_IOV
>> +static int pci_vf_memory_decoding(struct pci_dev *dev)
>> +{
>> +	u16 cmd;
>> +
>> +	pci_read_config_word(dev, dev->sriov->pos + PCI_SRIOV_CTRL, &cmd);
>> +	if (cmd & PCI_SRIOV_CTRL_MSE)
>> +		return -EBUSY;
>> +
>> +	return 0;
>> +}
>> +#endif
>> +
>>   int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>>   {
>>   	struct resource *res = dev->resource + resno;
>>   	struct pci_host_bridge *host;
>>   	int old, ret;
>>   	u32 sizes;
>> -	u16 cmd;
>>   
>>   	/* Check if we must preserve the firmware's resource assignment */
>>   	host = pci_find_host_bridge(dev->bus);
>> @@ -424,9 +447,14 @@ int pci_resize_resource(struct pci_dev *dev, int resno, int size)
>>   	if (!(res->flags & IORESOURCE_UNSET))
>>   		return -EBUSY;
>>   
>> -	pci_read_config_word(dev, PCI_COMMAND, &cmd);
>> -	if (cmd & PCI_COMMAND_MEMORY)
>> -		return -EBUSY;
>> +#ifdef CONFIG_PCI_IOV
>> +	if (resno >= PCI_IOV_RESOURCES)
>> +		ret = pci_vf_memory_decoding(dev);
>> +	else
>> +#endif
>> +	ret = pci_memory_decoding(dev);
>> +	if (ret)
>> +		return ret;
> 
> The above else works, however, it does trip our static analysis tools, and
> lack of the indentation makes it slightly confusing to read, at least at
> the first glance.  Thus, I wonder whether it would be possible to combine
> the pci_vf_memory_decoding() and pci_memory_decoding() somehow neatly into
> a private helper that takes a boolean to indicate whether it's a VF or not.
> Then, we could drop the if-statement, since some of the logic could live
> within the helper.
> 
> What do you think?

Sure - implementing a helper in a way that Christian suggested (the 
helper still takes resno though) should also take care of that.

Thanks
-Michał

> 
> 	Krzysztof