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;
Date:   Fri, 21 Jul 2023 15:49:50 +0200
From:   Robert Richter <rrichter@amd.com>
To:     Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
Cc:     Sathyanarayanan Kuppuswamy 
        <sathyanarayanan.kuppuswamy@linux.intel.com>,
        linux-pci@vger.kernel.org, linux-kernel@vger.kernel.org,
        linux-cxl@vger.kernel.org, Bjorn Helgaas <bhelgaas@google.com>,
        oohall@gmail.com, Lukas Wunner <lukas@wunner.de>,
        Mahesh J Salgaonkar <mahesh@linux.ibm.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Ben Widawsky <bwidawsk@kernel.org>,
        Dan Williams <dan.j.williams@intel.com>,
        Jonathan Cameron <Jonathan.Cameron@huawei.com>,
        Yazen Ghannam <yazen.ghannam@amd.com>,
        Terry Bowman <terry.bowman@amd.com>
Subject: Re: [PATCH 2/2] cxl/pci: Fix appropriate checking for _OSC while
 handling CXL RAS registers
Message-ID: <ZLqM/vi9lpXyNUQV@rric.localdomain>
References: <20230719192313.38591-1-Smita.KoralahalliChannabasappa@amd.com>
 <20230719192313.38591-3-Smita.KoralahalliChannabasappa@amd.com>
 <53d5eeb3-5a13-3663-57a1-e927c4c369b8@linux.intel.com>
 <937d872d-cbc1-3671-9c3d-ddceb9cb270b@amd.com>
 <ZLkxiZv3lWfazwVH@rric.localdomain>
 <fdadfe06-5d1e-978b-82bf-637a9ad9754a@amd.com>
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <fdadfe06-5d1e-978b-82bf-637a9ad9754a@amd.com>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?ksTLSpHucwycnL73aZOcqho3kXzYkuPMGPGJrFx/tjJFNoWAa3tL4WRgLGhQ?=
 =?us-ascii?Q?4aAe/CXryu1CuI8aDGJVRGHchELsL8+Scku6Bm3HLRLnNZ5+a8Ly2PyzVQl1?=
 =?us-ascii?Q?E67wW0CafNv/dea2J1yz4ImFiX2VuzhIos4Oe+3I14tzm6e2IUf+sUlHF2zK?=
 =?us-ascii?Q?+AzGKklZ4x2rCTvnmFZkucclhDut1LVXqt61x+sr+DcBIOHN+DyIpiE114OJ?=
 =?us-ascii?Q?aqrD1MsT1A7sCNdQMcEi/gSa682MQ3H24q8MCae2kqRinXT1OmVhOUOdOkBZ?=
 =?us-ascii?Q?hpWBciLghEXzgALKU9EofHmcnUPYxFxkSDuKFIaT4oW0Wp3DDGQC3r+n2MsN?=
 =?us-ascii?Q?ckJbtmYLj2srVIYQCgXC2h0AeGmT61HaR+nFtMNqoVycXKZDBeIbUn6TyOkH?=
 =?us-ascii?Q?KESUBUmuBUm+2ZjhpTpirNQOiUYS+3zEIFTH184GJQeirluWYyuK8Wd6rrPJ?=
 =?us-ascii?Q?cKXmZfEr/npD8807ogsawUN7oMS33KlPIa4w5ZwMTypp1HphptO2OXELGDig?=
 =?us-ascii?Q?JMqMsmUZTyvIR9/8TzuK2GwoR7JxbqUMcxsvsM1GjHPpypy9PJSIpLJYd0uB?=
 =?us-ascii?Q?7sP8on9mD6p8kE8/rR5fya+4i7siPe/JuGC8VhYhYecrGw5X6j1j9CA1ps1O?=
 =?us-ascii?Q?BtGzVltY5yfjW/N/CaFqdqz9lgW7cb8GjMSWTFxAQCqBG4aCWM3A68mmNk5V?=
 =?us-ascii?Q?cRlFFi6CKBs2yZKFaksUrinTykzmI7WOyUb5qCiF5lhAwMnvRpS1TIruqost?=
 =?us-ascii?Q?pYABSkh9ruy+rN5EwJk9U8HEBLvq0NtDZgz0oCn4Y1MjEo8HXRMSmuKkg738?=
 =?us-ascii?Q?FYVM3ZNDfFcFE7p3WIGTYacDDHxZxhqbsF/KB32BI0Q3Lhl6tTu/FWAOJS8N?=
 =?us-ascii?Q?ZjGC5ohfe47UG2jPWSQitL3l7M5pOUWLytSQ0qYCdy7eTJ3w+wSufwJhKb/x?=
 =?us-ascii?Q?7rrI8Vfz8d2ooksrZp9dTba+AyISFkLAz1dlwNPG+nImdtyptPSMggeaU/A5?=
 =?us-ascii?Q?68cabwy7bhHOvrrY0jq51R9nOD/mparVNG+5r4nXrQpRbaGqHdYGAl/WZfBf?=
 =?us-ascii?Q?d879FE58jnhmmju6O//3PuCfcjTEY9G/lG3eXC3prnNTyD8CQzBBp0i2lk5F?=
 =?us-ascii?Q?k+j0DYYRUmJTmK7s/aeR9D927PFgwWQyH9EZ9kn1gbZlaEcc2wTTbq7E1V+9?=
 =?us-ascii?Q?txmClITjLAAATcoiLpG4juQ1f3smqUWgLdAuWU3qAaI+0gmIIW/RtqgqYNYb?=
 =?us-ascii?Q?do+3m6Rm949+Y0ax/BPmf9Sbxxrs3x3YL4sTqW0wrg7cIDPEmW1yQHSc31uX?=
 =?us-ascii?Q?VEBTj+IV9omg9jm0LkKO8erz7IIxLeDU1VujauItvjRjm7XjVG2LDd9zFU9c?=
 =?us-ascii?Q?kB4Gp19yCbuYpcSujofQUIO1kYUB256tbAF+5Jf2yQK5UfA05X8l8rcoRCcp?=
 =?us-ascii?Q?paQTfDldreZY3HZ8XwcWMnlcC2sY5c1EiRo/PO4fpcN6MJeLN+AgQJDhS2Uz?=
 =?us-ascii?Q?+Vjas+zZW0lelJEEttv9ZowIIwKT6AqKWD9hGw5nvVFEofuY08WfPGzCs73X?=
 =?us-ascii?Q?jdBao1DvXOjunkur7B3oORrPN982WnESctNZROIW?=
X-OriginatorOrg: amd.com
X-MS-Exchange-CrossTenant-Network-Message-Id: 3bfe5bd3-f940-4174-b214-08db89f16d54
X-MS-Exchange-CrossTenant-AuthSource: DM6PR12MB3657.namprd12.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 21 Jul 2023 13:50:22.1424
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3dd8961f-e488-4e60-8e11-a82d994e183d
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: WWmcbiLRHNhyRpDdplejWTlJ8ydW8yw4s16vxgzevXJfKW0/IWuTHz0ZZwZlmV268I19Wt+jjdInrCc5I9I4kw==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: IA1PR12MB6042
X-Spam-Status: No, score=-1.1 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FORGED_SPF_HELO,
        RCVD_IN_DNSWL_BLOCKED,RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_NONE,
        T_SCC_BODY_TEXT_LINE,URIBL_BLOCKED autolearn=no 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 20.07.23 11:31:15, Smita Koralahalli wrote:
> On 7/20/2023 6:07 AM, Robert Richter wrote:
> > Smita,
> > 
> > On 19.07.23 15:30:25, Smita Koralahalli wrote:
> > > On 7/19/2023 1:39 PM, Sathyanarayanan Kuppuswamy wrote:
> > > > 
> > > > 
> > > > On 7/19/23 12:23 PM, Smita Koralahalli wrote:
> > > > > According to Section 9.17.2, Table 9-26 of CXL Specification [1], owner
> > > > > of AER should also own CXL Protocol Error Management as there is no
> > > > > explicit control of CXL Protocol error. And the CXL RAS Cap registers
> > > > > reported on Protocol errors should check for AER _OSC rather than CXL
> > > > > Memory Error Reporting Control _OSC.
> > > > > 
> > > > > The CXL Memory Error Reporting Control _OSC specifically highlights
> > > > > handling Memory Error Logging and Signaling Enhancements. These kinds of
> > > > > errors are reported through a device's mailbox and can be managed
> > > > > independently from CXL Protocol Errors.
> > > > 
> > > > Does it fix any issue? If yes, please include that in the commit log.
> > > 
> > > Yes, this fix actually makes Protocol Error handling independent of
> > > Component/Memory Error handling.
> > > 
> > > We observed that OS was not able to handle the protocol errors ("i.e unable
> > > to reference to the cxl device node") with native AER support. The reason
> > > being Memory/Component Error handling was under FW control.
> > > 
> > > Since the RAS registers are tied to protocol errors, I think there is no
> > > reason that memory error reporting being in fw control or os control should
> > > be a roadblock in handling RAS registers or accessing the cxl device node by
> > > OS.
> > > 
> > > > 
> > > > Since you are removing some change, maybe it needs Fixes: tag?
> > > 
> > > Missed this. Thanks!
> > > 
> > > Fixes: 248529edc86f ("cxl: add RAS status unmasking for CXL")
> > 
> > the fix must be isolated to this patch (for automated backports) and
> > you need to remove the dependency to the first patch then. So swap
> > them and ... see below.
> > 
> > > 
> > > Will include in v2.
> > > 
> > > Thanks,
> > > Smita
> > > 
> > > > > 
> > > > > [1] Compute Express Link (CXL) Specification, Revision 3.1, Aug 1 2022.
> > > > > 
> > > > > Signed-off-by: Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>
> > > > > ---
> > > > >    drivers/cxl/pci.c | 7 +++----
> > > > >    1 file changed, 3 insertions(+), 4 deletions(-)
> > > > > 
> > > > > diff --git a/drivers/cxl/pci.c b/drivers/cxl/pci.c
> > > > > index 1cb1494c28fe..44a21ab7add5 100644
> > > > > --- a/drivers/cxl/pci.c
> > > > > +++ b/drivers/cxl/pci.c
> > > > > @@ -529,7 +529,6 @@ static int cxl_pci_setup_regs(struct pci_dev *pdev, enum cxl_regloc_type type,
> > > > >    static int cxl_pci_ras_unmask(struct pci_dev *pdev)
> > > > >    {
> > > > > -	struct pci_host_bridge *host_bridge = pci_find_host_bridge(pdev->bus);
> > > > >    	struct cxl_dev_state *cxlds = pci_get_drvdata(pdev);
> > > > >    	void __iomem *addr;
> > > > >    	u32 orig_val, val, mask;
> > > > > @@ -541,9 +540,9 @@ static int cxl_pci_ras_unmask(struct pci_dev *pdev)
> > > > >    		return 0;
> > > > >    	}
> > > > > -	/* BIOS has CXL error control */
> > > > > -	if (!host_bridge->native_cxl_error)
> > 
> > For the fix, you could replace that with:
> > 
> > 	if (!host_bridge->native_aer) ...
> 
> Yeah I tried something like:
> +	if (!pdev->aer_cap &&
> +	    !(pcie_ports_native || host_bridge->native_aer))
> +		return 0;
> 
> But then pcie_ports_native needed to be exported as well. So better just
> keep the check to !host_bridge->native_aer and return zero in first patch,
> EXPORT to second and replacing host_bridge->native_aer with
> pcie_aer_is_native() in third?

Looks good.

Thanks,

-Robert

> 
> Thanks,
> Smita
> 
> > 
> > > > > -		return -ENXIO;
> > > > > +	/* BIOS has PCIe AER error control */
> > > > > +	if (!pcie_aer_is_native(pdev))
> > > > > +		return 0;
> > 
> > ... and replace it with this function here in the patch where
> > pcie_aer_is_native() is exported (or in a 3rd patch).
> > 
> > -Robert
> > 
> > > > >    	rc = pcie_capability_read_word(pdev, PCI_EXP_DEVCTL, &cap);
> > > > >    	if (rc)
> > > > 
> > > 
>