Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <18a48407-9fd7-175d-0c7f-5702b077f13d@nvidia.com>
Date:   Tue, 11 Oct 2022 17:47:50 +0100
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.2.2
Subject: Re: [PATCH] PCI: tegra: Use PCI_CONF1_EXT_ADDRESS() macro
Content-Language: en-US
To:     =?UTF-8?Q?Pali_Roh=c3=a1r?= <pali@kernel.org>
Cc:     Thierry Reding <thierry.reding@gmail.com>,
        Lorenzo Pieralisi <lpieralisi@kernel.org>,
        Rob Herring <robh@kernel.org>,
        =?UTF-8?Q?Krzysztof_Wilczy=c5=84ski?= <kw@linux.com>,
        Bjorn Helgaas <bhelgaas@google.com>,
        Vidya Sagar <vidyas@nvidia.com>, linux-tegra@vger.kernel.org,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20220928121911.14994-1-pali@kernel.org>
 <220b0fe7-8b7b-cf9b-e28d-d9d81647fb80@nvidia.com>
 <20221011161638.ycxpg3ox2wv63vym@pali>
From:   Jon Hunter <jonathanh@nvidia.com>
In-Reply-To: <20221011161638.ycxpg3ox2wv63vym@pali>
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?TnpqekRNbnVZTk5CUUdMV1NSNC94UHdHT2lraXFRV1huWVd5NnVyR3gvOXZY?=
 =?utf-8?B?WC9YWXp3WDdYT0ZQOTU4elNvNGJONFhkN3pFMzVqa2IveitDb2dITnYvanUr?=
 =?utf-8?B?YSt1Y1FjVHd4NFRhZUR0dkpBbWVDcU9TWkZoWGw5V2NXMGgyWDBLM1IzSkU0?=
 =?utf-8?B?NUUwTkdWS0REK2tKRUdoZ2swWGZJMWFBYkIvQzBWdzV5WE1KaWYxWkowSDhC?=
 =?utf-8?B?OUxjWFltVUxFd3NvTUIvNURDZitEMmFZQ09YR3pYUzk2NkRtYzZEcDJna3c2?=
 =?utf-8?B?Y1lqckY0bmJVTXN6Mk9yd3R0L3NoaS9HWlJ6cWlkTERHeGtUbGtsT2RlMVNC?=
 =?utf-8?B?UkhUQ0hlTlNCNG05NlluZVFXamxPaFVxSGJ4aHc3bkF0amZUY1pXdndtRGp5?=
 =?utf-8?B?Slh4T3VWWldtYXhjRW9CdjdjNXhtMDdkRjk0b1dlY2Q1eE95am1leEo5MmVM?=
 =?utf-8?B?SnlobVlBRmVkRE5rdmdrVzhSRHJJVEZFVzBPcXd0R09yelRsdml6ZzZjRmt4?=
 =?utf-8?B?S0J0WTljQkJ1UXMrRFV5cDNVeHNXWGV6VUt5YnZVaWNJSUMvay9IbGtDODVo?=
 =?utf-8?B?STB1ZlRxL0dlanl2djFrOHdNNll1dlBmK0F6OTl0OThwTC9lTzBZcjBTUnhP?=
 =?utf-8?B?TVh6TEpIUXA1bHh6bFRoc2xsT3hBZTVFWVpLK3JtSFJnRVRZRDdHM1FnL0Mw?=
 =?utf-8?B?YjZBdzBCSG5WYWwyamhPaklvZk5nVnMrOEF6aXVSQzJIYXBiUTAwOGs3U004?=
 =?utf-8?B?TjVBWHpxbldPU1BRQ05iUzdLWnpJcnBmaEdyeFM1Sk1VbUZQVVhtOUdyMVAv?=
 =?utf-8?B?alRhNHJuSERaNzMzcDdHVmhaVFlIdHNweWpaUFYwUEhjN002NkpXbnZvOW5w?=
 =?utf-8?B?ZUlId1lWbUl1dGpHTkEyQWpzSzBFRitIbmFCQisrMm14cGF4bVQvYm1neUxY?=
 =?utf-8?B?UFVrdC9mVEs1ZEdIbmN2RUorcTVkWmFJNWN5MG5pN2M1SGkxZG92aDA3UW5Q?=
 =?utf-8?B?VFR2YU10a2poZVF5Y1dzOStiRzB3QTkzTGZRL1pzNlVjZXEydWd2WHE3MkJD?=
 =?utf-8?B?VGJpZHVLNEZBRWdtSnowOHRkU0FIYlNoSDZucEZrSmZva01pYk50Y2lXVE9O?=
 =?utf-8?B?QlZqaHZvTWdMQmFSaHJBY1JWQncvaUczS05PTmRhNVExVUdhNnRpeUFHODhH?=
 =?utf-8?B?eU1pSGRDMm9nemxERzZaWUpsQ0tZNTBlK3E2cUE3YjVrRkZlL1pIMndoaDdO?=
 =?utf-8?B?QU50Z2VGeEUxL3ZWai8vVXZaa0tFRzJUZ2hBNEFGTlVwUHJYTHpBY3cxZDhz?=
 =?utf-8?B?VEFVYTZ5eE5icTVWN0dOSkJBNVV5UjI4d0ozRnlLV1RCMEJuRGc1d3UxM3Nt?=
 =?utf-8?B?dE5GMkdmcUpDMzdZUHppN3dQc3RoeVN1VVJ4UmRPTzdmNDRIU1BaeWhOZnhW?=
 =?utf-8?B?Y0k5cldSUURmOW9lL2NHM3NBUkJzb1Y0eUd1c3J3ODJZRFlQR3dEQVdrK05x?=
 =?utf-8?B?bFRqb20zbVBFRVFOZ2tnYnIvZUxLbzFTb2pVM3J0cVA5VDlvQ0djVmU2U2pl?=
 =?utf-8?B?cFdtcjQ0S3E3T0lJV0Fsdkh3T3NKMFptSFFmYXJ3Y08yYk9sMU8zM1ZEZWRI?=
 =?utf-8?B?NFA4QUExTkdaaUduRER2VVRSYmpjbml1UFREK1EyNDJLYWorYzJRMHNQVFZx?=
 =?utf-8?B?RVNBbHZKNVpzU0JoR1hLQzVqN3BTZGhGUkRXNXByUVVkZ2E5SERzTGtFdFRz?=
 =?utf-8?B?dlRlUElwL3R0OHE2VVlMMjB3dXNoR0V6d3lvbFBsWTJYTE5LVXdkb0tQZGVL?=
 =?utf-8?B?cFBSejlmZVJTVi9hcm9oOU1yL0JFWXJoNmc5SkhWc2d3S3dQQ3RBK2VvM3dE?=
 =?utf-8?B?aFkzSU9OWmM2d1Fwa1ZLZUtQUXdwTUFSSXhMOWw1d3k3S0dRVjd4M2ZRSG05?=
 =?utf-8?B?bzNlYi9CMm5hdW1tcUk3Q2V2WUJrK0dPOE1lQTlSTEtlekNtd2VDNUlwZXlF?=
 =?utf-8?B?RG52RGx3U2VPb1VobCtEMmJ4N0VXR2M3WFUyNU5iVHkwNGlCVHBVeVdldE5k?=
 =?utf-8?B?cjduTUdyM3Z6Y3h6cmQ2NEN0MmlBZjhRbys5Vk0xRjIrcXkrRkJJbnNVMHdi?=
 =?utf-8?B?UktFT1p2Qk9FemsrRlFwWlNWc01NYXBYMEtKK2R6UllFdDRTSTJWd3ZtRW9i?=
 =?utf-8?B?Tmc9PQ==?=
X-OriginatorOrg: Nvidia.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 6753465f-2e2b-4fdb-6b8d-08daaba859dd
X-MS-Exchange-CrossTenant-AuthSource: CO6PR12MB5444.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 11 Oct 2022 16:47:57.9853
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 43083d15-7273-40c1-b7db-39efd9ccc17a
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: EV4eAZ8AaJTYWrOvmBmQafbxSkfV6y1a8PKVylXFGiHUPmTJrTrBQOLLo0wYKI/SXkOgNmxPlTz6qHma1iDytw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: SN7PR12MB7225
X-Spam-Status: No, score=-4.0 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        NICE_REPLY_A,RCVD_IN_DNSWL_NONE,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,
        SPF_NONE autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org


On 11/10/2022 17:16, Pali Rohár wrote:

...

> I see, this is stupid mistake. PCIe config read and write operations
> needs to be 4-byte aligned, so normally it is done by calculating 4-byte
> aligned base address and then using appropriate cpu load/store
> instruction to access just defined size/offset of 4-byte config space
> register.
> 
> pci-tegra.c is using common helper functions pci_generic_config_read()
> and pci_generic_config_write(), which expects final address with offset,
> and not 4-byte aligned address.
> 
> I'm not sure what should be the proper fix, but for me it looks like
> that pci_generic_config_read() and pci_generic_config_write() could be
> adjusted to handle it.
> 
> In any case, above patch is a regressions and I see there two options
> for now:
> 
> 1) Reverting that patch
> 
> 2) Adding "offset |= where & 0x3;" after the PCI_CONF1_EXT_ADDRESS()
>     macro to set also lower 2 bits of accessed register.
> 
> Jon, Lorenzo, what do you think? Could you test if 2) is working fine?


I tested 'offset |= where & 0xff' which is essentially the same as the 
above and that is working and so I am sure that the above works too. 
However, I do wonder if reverting is simpler because we already have a 
'& ~PCI_CONF1_ENABLE' and now adding '| where & 0x3' seems to diminish 
the value of this change.

Cheers
Jon

-- 
nvpublic