Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
Date:   Fri, 8 Dec 2023 11:43:42 -0800
From:   Ira Weiny <ira.weiny@intel.com>
To:     Dan Williams <dan.j.williams@intel.com>,
        Ira Weiny <ira.weiny@intel.com>,
        Jonathan Cameron <jonathan.cameron@huawei.com>,
        Smita Koralahalli <Smita.KoralahalliChannabasappa@amd.com>,
        Shiju Jose <shiju.jose@huawei.com>
CC:     Yazen Ghannam <yazen.ghannam@amd.com>,
        Davidlohr Bueso <dave@stgolabs.net>,
        Dave Jiang <dave.jiang@intel.com>,
        Alison Schofield <alison.schofield@intel.com>,
        Vishal Verma <vishal.l.verma@intel.com>,
        "Ard Biesheuvel" <ardb@kernel.org>, <linux-efi@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>, <linux-cxl@vger.kernel.org>,
        Ira Weiny <ira.weiny@intel.com>
Subject: Re: [PATCH 6/6] cxl/memdev: Register for and process CPER events
Message-ID: <657371eec6ac5_1e7d272948d@iweiny-mobl.notmuch>
References: <20230601-cxl-cper-v1-0-d19f1ac18ab6@intel.com>
 <20230601-cxl-cper-v1-6-d19f1ac18ab6@intel.com>
 <657279a68c270_b991294e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
Content-Type: text/plain; charset="us-ascii"
Content-Disposition: inline
In-Reply-To: <657279a68c270_b991294e@dwillia2-mobl3.amr.corp.intel.com.notmuch>
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?us-ascii?Q?p0RwUSqay91RJ+3FCR6YivK9fsVf+yKbD2I5jw4wPScXz3XGNyDl4o+iVvTH?=
 =?us-ascii?Q?o8NORaxIix9a8gCEjBjCWZDYcBo//UWO8Hzo3p2d3rfKdb+EEAtr+HN7f8wk?=
 =?us-ascii?Q?P4Pvtu1CFWWU+3nUG+PsYW9UuYcRNGjQeLIFumAQKWOX7ux4n0vSH6WDUt/4?=
 =?us-ascii?Q?zXBNxXUBVRU7FuK7ApVWBx4/Vs5uSPkt4u46J8oEjPwdRWVboYJqV4qNkSfv?=
 =?us-ascii?Q?yyQUZkAVovkpKvkNl/6V4e1BxlwMwiclbJDaHuyhUek0SQgdqddb5L1h80tZ?=
 =?us-ascii?Q?hcRJxn5qIKulbYZt+9JU1oZxRTM+o5lLJhhtxYwr7rrQM09oyfscvyIzg7JR?=
 =?us-ascii?Q?M9/WvqeEH1zV4ML5+wGllhFAx7ePJslvl9NKwm0/rUxMuNKyqH2wyhjeIrzg?=
 =?us-ascii?Q?lJsAeyL3XFY6rteckFpKUPLwkzScbsAxXcChF4XzsHTurZnNXETT1q4A3d7/?=
 =?us-ascii?Q?V8swVPuRfxrhojIJswRiCkwXApZv0WvRtUzoNzbxK7HhqO1ksOAsjpGpvk7Q?=
 =?us-ascii?Q?Xu1/U/OCpPbgxPVY6Sz7dxklq5X3CzOH0fcntWWYKii/J82UOaj7mmMP3oLQ?=
 =?us-ascii?Q?oBo6M4PTg9uMqvffpgTpYqjnAZzApcxmZlzpru9ZzoCZxNdKTHCcb46wRu9v?=
 =?us-ascii?Q?IEkcY83nf+mjA/CUY1cVrCgHoIAcPR6yMfH7S5MIh9DpSiqo26scsm6v4YBj?=
 =?us-ascii?Q?o+WOKnnqTmWXJ7kivNQfQU4IleNB/ic9P4OaDCLtUnN6Q2OpYtkM6nIBs/vs?=
 =?us-ascii?Q?tfoc61WJNhaWviZCvpg7RBzLyp0yBOCak8MAcNYNc4fvy6HdRjBYW+3kqC8h?=
 =?us-ascii?Q?MGwuv1UF9g7jKfEKSP0d7VJrXl+ZWvIAZAgysMe3FKkgLZjTErPoseYCUfJe?=
 =?us-ascii?Q?BDbqXW2SIxd0/i34JKaBZfjHFWFV98YCJUboyx9/uR4AeiOphVh1xc4ABPVr?=
 =?us-ascii?Q?9yrBkG5kqM1cNldzRa6vxvrlKsympbyW+bkIaFXy47V//5NgjgdwNbbdiYxr?=
 =?us-ascii?Q?ve7YeOCQ9/sZRD+BY/TOeTQmbpBI5ZEDi0eM/SNzWGB28IxFPOQYoGV8dc4g?=
 =?us-ascii?Q?PtNmFKp4tlEWjnaZCD/GPFr+lub7tjjvbT4VZmhihFBru1zgBwpRfJ5Fm9ph?=
 =?us-ascii?Q?OC5WyEnMSfdj/aF3UfHpBaXPlGBmdH/zH2YIp/eEmCoOC3XUawJyfjfqRW4m?=
 =?us-ascii?Q?YtgUDG2bhDV5lbI2SFu0gJJre2BR2p4srQIaAR17Jv7S7bMmYZS3qPicOtYK?=
 =?us-ascii?Q?DBRVIOfG6guSU1K49zdiZyC8iO1hX2FkZ49zR4jhMvviiwfhLRbCzURewzED?=
 =?us-ascii?Q?hRzlCkaegFoxyy9hoNnQ575M5td0lPWJxmeZUy9Y3Ge4L0OT8IaB76M0Nb0o?=
 =?us-ascii?Q?fa/9fdS0bPzRASY4KVgtdJqSK9eLsNtYPXK1M7WI51IjKuLtjHRlyZemcr2y?=
 =?us-ascii?Q?GVgbmtDGSCtq4aYzl79aoPd9r5neO2WRHs7q3JYPJT+IoANTBilYNUPphknR?=
 =?us-ascii?Q?iD+eJjW2IKrBIz7Fvu6ziBnpFNYr7WbmPVWE7gIbggcrxfFMKO4PY82j2et6?=
 =?us-ascii?Q?9i/0STm9h7jdy6Z4ndKo7SaM/bbN0Sz9PpCo4iTZ?=
X-MS-Exchange-CrossTenant-Network-Message-Id: 746fac1e-67c6-4d9d-345d-08dbf825fe79
X-MS-Exchange-CrossTenant-AuthSource: SA1PR11MB6733.namprd11.prod.outlook.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 08 Dec 2023 19:43:47.1966
 (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: /mxo9fFFBqt87dSIlwgAOiYwrfupkkp48KfTltlkqoPs9BFVmkTo86VS9DSRfic26XCEmlp8G9Ww4zsSHLycJg==
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR11MB5619
X-OriginatorOrg: intel.com
X-Spam-Status: No, score=-4.4 required=5.0 tests=BAYES_00,DKIMWL_WL_HIGH,
        DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,RCVD_IN_DNSWL_MED,
        SPF_HELO_NONE,SPF_NONE,T_SCC_BODY_TEXT_LINE 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
X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (snail.vger.email [0.0.0.0]); Fri, 08 Dec 2023 11:43:58 -0800 (PST)

Dan Williams wrote:
> Ira Weiny wrote:

[snip]

> >  
> > +#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
> > +static int cxl_cper_event_call(struct notifier_block *nb, unsigned long action,
> > +			       void *data)
> > +{
> > +	struct cxl_cper_notifier_data *nd = data;
> > +	struct cper_cxl_event_devid *device_id = &nd->rec->hdr.device_id;
> > +	enum cxl_event_log_type log_type;
> > +	struct cxl_memdev_state *mds;
> > +	struct cxl_dev_state *cxlds;
> > +	struct pci_dev *pdev;
> > +	unsigned int devfn;
> > +	u32 hdr_flags;
> > +
> > +	mds = container_of(nb, struct cxl_memdev_state, cxl_cper_nb);
> > +
> > +	devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
> > +	pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
> > +					   device_id->bus_num, devfn);
> > +	cxlds = pci_get_drvdata(pdev);
> > +	if (cxlds != &mds->cxlds) {
> 
> Checks of drvdata are only valid under the device lock, or with the
> assumption that this callback will never be called while pci_get_drvdata
> would return NULL.

For the device we have registered pci_get_drvdata() will be always be valid.
Each driver is registering it's own call with valid driver state in the chain.

However, I see I have a bug here.  Using devm_add_action_or_reset() breaks
this assumption.

> 
> With that, the check of cxlds looks like another artifact of using a
> blocking notifier chain for this callback.

It is a desired artifact.  This check is determining if this event is for this
device.  It is not checking if cxlds is valid.

> With an explicit single
> callback it simply becomes safe to assume that it is being called back
> before unregister_cxl_cper() has run. I.e. it is impossible to even
> write this check in that case.

Exploring the use of a single register call...  you must check if the cxlds is
valid on that pdev.  Because the driver may not be attached.

Something like this in cxl_core vs cxl_pci:

#define CXL_EVENT_HDR_FLAGS_REC_SEVERITY GENMASK(1, 0)
static void cxl_cper_event_call(struct cxl_cper_notifier_data *nd)
{       
        struct cper_cxl_event_devid *device_id = &nd->rec->hdr.device_id;
        enum cxl_event_log_type log_type;
        struct cxl_dev_state *cxlds;
        struct pci_dev *pdev;
        unsigned int devfn;
        u32 hdr_flags;

        devfn = PCI_DEVFN(device_id->device_num, device_id->func_num);
        pdev = pci_get_domain_bus_and_slot(device_id->segment_num,
                                           device_id->bus_num, devfn);
        device_lock(&pdev->dev);
        cxlds = pci_get_drvdata(pdev);
        if (!cxlds)
                goto out;
        
        /* Fabricate a log type */
        hdr_flags = get_unaligned_le24(nd->rec->event.generic.hdr.flags);
        log_type = FIELD_GET(CXL_EVENT_HDR_FLAGS_REC_SEVERITY, hdr_flags);
        
        cxl_event_trace_record(cxlds->cxlmd, log_type, nd->event_type,
                               &nd->rec->event);
out:    
        device_unlock(&pdev->dev);
        pci_dev_put(pdev);
}

This does simplify registering.

Is this what you were thinking?

[snip]

> > +
> > +static void register_cper_events(struct cxl_memdev_state *mds)
> > +{
> > +	mds->cxl_cper_nb.notifier_call = cxl_cper_event_call;
> > +
> > +	if (register_cxl_cper_notifier(&mds->cxl_cper_nb)) {
> > +		dev_err(mds->cxlds.dev, "CPER registration failed\n");
> > +		return;
> > +	}
> > +
> > +	devm_add_action_or_reset(mds->cxlds.dev, cxl_unregister_cper_events, mds);
> 
> Longer term I am not sure cxl_pci should be doing this registration
> directly to the CPER code vs some indirection in the core that the
> generic type-3 and the type-2 cases can register for processing. That
> can definitely wait until a Type-2 CXL.mem device driver arrives and
> wants to get notified of CXL CPER events.
> 

Yes these calls will need to be moved to the core for drivers to share
later.  Same for mailbox event handling.

Ira