Received: by 2002:a05:6a10:f347:0:0:0:0 with SMTP id d7csp102103pxu; Sun, 22 Nov 2020 01:12:58 -0800 (PST) X-Google-Smtp-Source: ABdhPJzJ0MwScprIeEh4Om5xpfvE2pGbGGkcTDpsJF/Hds0xhKcpcqscERh4+tSN9VDJvmt/E6pG X-Received: by 2002:a17:906:268c:: with SMTP id t12mr38106699ejc.377.1606036377857; Sun, 22 Nov 2020 01:12:57 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1606036377; cv=none; d=google.com; s=arc-20160816; b=tcEM7YYyyNvOnnNawdleUS43mmMWmiGiralqgJeabWGT5HhHewEz1+tD+wwpqBVxaU gx5esGeO97MvMRHPv0meuNkt/PGuK0Jy88P9IMg+iOb+V9JZSxMC985JKyuQVqQ65RvE L3kLpvy7MOkxQTxNqeOAW58rlbR80SEFKHBVlzTZjad1d3XXiSKmqEiwOkzqkIyWFYhi jMt2McZwlC1TYcM0xeS4O9k0mbl38jVXuUIYSHzcw6qf1ojTUiUZpXT6KwH74IT0GTAG y+d6p7o8Mml57MUcLb3KBH/jHp12MH9qvxG/lMDKQDT7a34j0/pVeV8+Y4VDmDF+VUKm R6bQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:user-agent:in-reply-to:content-disposition :mime-version:references:message-id:subject:cc:to:from:date; bh=yOZlfGnPDt5tJROgJrwbXWu0HA9PmLHFYJzLagk7YXw=; b=uHXzDv2cWRbvTU+nh05emJqQiszsZfs4zOlN9KSFTZFXmDg3E+XUdwYtNSjHSchnud up6ngn3a9ms78/lbmtWA7RXgXHWBhF2qSHhy3j90skjHsubo5JkCvkwC5oRq7IM3KuGw zAw+8S+kbJYDUYZs+ByjUHHgMI3wIYpDTwxGlaRUbeVG79vvh8PPySg2ugzzv5nEe3/O iDPYhyqV94F7+zFPnW+cjMEzj7wMqWOZlypgf3RESDYcSG9p+bVYUdeXMJ7hPXs9d9ee yyLcEb3+akhZcEvglvTZ+mIfEDoLM2OGi3P1WAiz/CjzSonJ81Uh5A/gGeYTnT3G9c8m WhIw== ARC-Authentication-Results: i=1; mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id g17si4919987edy.62.2020.11.22.01.12.21; Sun, 22 Nov 2020 01:12:57 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1727366AbgKVJI6 (ORCPT + 99 others); Sun, 22 Nov 2020 04:08:58 -0500 Received: from bmailout3.hostsharing.net ([176.9.242.62]:51837 "EHLO bmailout3.hostsharing.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1727317AbgKVJI6 (ORCPT ); Sun, 22 Nov 2020 04:08:58 -0500 Received: from h08.hostsharing.net (h08.hostsharing.net [83.223.95.28]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (Client CN "*.hostsharing.net", Issuer "COMODO RSA Domain Validation Secure Server CA" (not verified)) by bmailout3.hostsharing.net (Postfix) with ESMTPS id 00E7B100E417C; Sun, 22 Nov 2020 10:08:51 +0100 (CET) Received: by h08.hostsharing.net (Postfix, from userid 100393) id 4D14923339E; Sun, 22 Nov 2020 10:08:52 +0100 (CET) Date: Sun, 22 Nov 2020 10:08:52 +0100 From: Lukas Wunner To: Ashok Raj Cc: Bjorn Helgaas , linux-pci@vger.kernel.org, Sathyanarayanan Kuppuswamy , linux-kernel@vger.kernel.org Subject: Re: [Patch v2 1/1] PCI: pciehp: Add support for handling MRL events Message-ID: <20201122090852.GA29616@wunner.de> References: <20201122014203.4706-1-ashok.raj@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20201122014203.4706-1-ashok.raj@intel.com> User-Agent: Mutt/1.10.1 (2018-07-13) Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Sat, Nov 21, 2020 at 05:42:03PM -0800, Ashok Raj wrote: > --- a/drivers/pci/hotplug/pciehp_ctrl.c > +++ b/drivers/pci/hotplug/pciehp_ctrl.c > void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events) > { > int present, link_active; > + u8 getstatus = 0; > > /* > * If the slot is on and presence or link has changed, turn it off. > @@ -246,6 +259,20 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events) > if (events & PCI_EXP_SLTSTA_PDC) > ctrl_info(ctrl, "Slot(%s): Card not present\n", > slot_name(ctrl)); > + if (events & PCI_EXP_SLTSTA_MRLSC) > + ctrl_info(ctrl, "Slot(%s): Latch %s\n", > + slot_name(ctrl), getstatus ? "Open" : "Closed"); This message will currently always be "Latch closed". It should be "Latch open" instead because if the slot was up, the latch must have been closed. So an MRLSC event can only mean that the latch is now open. The "getstatus" variable can be removed. > + /* > + * PCIe Base Spec 5.0 Chapter 6.7.1.3 states. > + * > + * If an MRL Sensor is implemented without a corresponding MRL Sensor input > + * on the Hot-Plug Controller, it is recommended that the MRL Sensor be > + * routed to power fault input of the Hot-Plug Controller. > + * This allows an active adapter to be powered off when the MRL is opened." > + * > + * This seems to suggest that the slot should be brought down as soon as MRL > + * is opened. > + */ > pciehp_disable_slot(ctrl, SURPRISE_REMOVAL); > break; The code comment is not wrapped at 80 chars and a bit long. I'd move it to the commit message and keep only a shortened version here. The "SURPRISE_REMOVAL" may now be problematic because the card may still be in the slot (both presence and link still up) with only the MRL open. My suggestion would be to add a local variable "bool safe_removal" which is initialized to "SAFE_REMOVAL". In the two if-clauses for DLLSC and PDC, it is set to SURPRISE_REMOVAL. > @@ -275,6 +302,13 @@ void pciehp_handle_presence_or_link_change(struct controller *ctrl, u32 events) > if (link_active) > ctrl_info(ctrl, "Slot(%s): Link Up\n", > slot_name(ctrl)); > + /* > + * If slot is closed && ATTN button exists > + * don't continue, let the ATTN button > + * drive the hot-plug > + */ > + if (((events & PCI_EXP_SLTSTA_MRLSC) && ATTN_BUTTN(ctrl))) > + return; > ctrl->request_result = pciehp_enable_slot(ctrl); > break; Hm, if the Attention Button is pressed with MRL still open, the slot is not brought up. If the MRL is subsequently closed, it is still not brought up. I guess the slot keeps blinking and one has to push the button to abort the operation, then press it once more to attempt another slot bringup. The spec doesn't seem to say how such a situation should be handled. Oh well. I'm wondering if this is the right place to bail out: Immediately before the above hunk, the button_work is canceled, so it can't later trigger bringup of the slot. Shouldn't the above check be in the code block with the "Turn the slot on if it's occupied or link is up" comment? You're also not unlocking the state_lock here before bailing out of the function. > @@ -710,8 +710,10 @@ static irqreturn_t pciehp_ist(int irq, void *dev_id) > down_read(&ctrl->reset_lock); > if (events & DISABLE_SLOT) > pciehp_handle_disable_request(ctrl); > - else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC)) > + else if (events & (PCI_EXP_SLTSTA_PDC | PCI_EXP_SLTSTA_DLLSC | > + PCI_EXP_SLTSTA_MRLSC)) > pciehp_handle_presence_or_link_change(ctrl, events); > + > up_read(&ctrl->reset_lock); Unnecessary newline added. > @@ -768,6 +770,14 @@ static void pcie_enable_notification(struct controller *ctrl) > cmd |= PCI_EXP_SLTCTL_ABPE; > else > cmd |= PCI_EXP_SLTCTL_PDCE; > + > + /* > + * If MRL sensor is present, then subscribe for MRL > + * Changes to be notified as well. > + */ > + if (MRL_SENS(ctrl)) > + cmd |= PCI_EXP_SLTCTL_MRLSCE; > + The code comment doesn't add much information, so can probably be dropped. You need to add PCI_EXP_SLTCTL_MRLSCE to the "mask" variable in this function (before PFDE, as in pcie_disable_notification()). I don't think the interrupt is enabled at all if it's not added to "mask", has this patch been tested at all? Something else: When pciehp probes, it should check whether the slot is up even though MRL is open. (E.g. the machine is booted, the card in the slot was enumerated but the latch is open.) I think in that case we need to bring down the slot. I suggest adding a check to pciehp_check_presence() whether the latch is open. If so, a PCI_EXP_SLTSTA_MRLSC event should be synthesized. You could rename the latch_closed() function to pciehp_latch_closed() and remove its "static" attribute so that you can call it from pciehp_core.c. Thanks, Lukas