From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.14]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 3832A16DC29 for ; Mon, 22 Jul 2024 14:21:31 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.14 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721658093; cv=none; b=Hq6p5we/km1tKSJKHYcfBXOXxGyWdTmF4QXU+IE6viLGJ4yMQvUp32qFeAPqDOVXL9mKFgkPBmMVqM9e2wrNfJZLe9pfqRpBGzZ0+0FRTF3ejCKDqaC0r6YeyJRmz9zTqHLV4q/8uBUXauBeaFroHD9e/S+wLUhXAXV7yO2iVzg= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1721658093; c=relaxed/simple; bh=f2KnnQddRwFfgtBjTaHiOv7R3ff9IenpvZIgjdFmqSg=; h=Date:From:To:Cc:Subject:Message-ID:In-Reply-To:References: MIME-Version:Content-Type; b=rdXuSMZHk7/zUIX0QX9YxcypNmvllu4bxFJmV4QihM7B+szr2yEtUvMgfWPZRMgNQ2TvZmMfMSufQ3m+eVOI2zzS2hhlB2r5ZsnDpLFfbSrj6OcexTIJD02qXKUTbSirOmcgP+UVxdxumLn6nnlGrZoE4LQ8H/i1kRnoPvWpfmA= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com; spf=none smtp.mailfrom=linux.intel.com; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b=FKOxn42F; arc=none smtp.client-ip=192.198.163.14 Authentication-Results: smtp.subspace.kernel.org; dmarc=pass (p=none dis=none) header.from=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; spf=none smtp.mailfrom=linux.intel.com Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=intel.com header.i=@intel.com header.b="FKOxn42F" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1721658092; x=1753194092; h=date:from:to:cc:subject:message-id:in-reply-to: references:mime-version:content-transfer-encoding; bh=f2KnnQddRwFfgtBjTaHiOv7R3ff9IenpvZIgjdFmqSg=; b=FKOxn42FKcNNZR2R9gpyQIzuk+KysPhfBWrVDrBHWcuQ/ctwCgF43PBK I1+527hqVxCojo/oT0meSAJS9Y94kUc9vzDIkfBjB+gGj6DH3j+Ee6MAO 5eGvg3IyjAoP6qGdi6+Q6iFywJdaDYO43RZKZ6VvBPDXcWjnt0mfePhwt 2nDPfAHJX6H6kc5GM8hSO+5nBvH/Pk1D/6nve0Zux4njOwZ5eQKDeYmQH jjwDl5NshEwH+LR1y3ZkqsNqMMCFELn+tfUkpEVTjZY//mdySIiEbI79G brVYFuj+Ozs9+nvS8BCABuaSQONmDzi1hdueZqogDk++IaWwiSPc7k7Uj w==; X-CSE-ConnectionGUID: 2bu1nB6LQTuG/bf6LLHqEA== X-CSE-MsgGUID: wRmYAzI7RGa+lKEpM5tfpg== X-IronPort-AV: E=McAfee;i="6700,10204,11141"; a="19413579" X-IronPort-AV: E=Sophos;i="6.09,228,1716274800"; d="scan'208";a="19413579" Received: from fmviesa002.fm.intel.com ([10.60.135.142]) by fmvoesa108.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jul 2024 07:21:31 -0700 X-CSE-ConnectionGUID: 5Gs7hyFASkOZtTPQSykKcA== X-CSE-MsgGUID: pC8xDfUwQSWNbOUlYTQJxQ== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.09,228,1716274800"; d="scan'208";a="75106381" Received: from unknown (HELO localhost) ([10.237.142.103]) by fmviesa002-auth.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 22 Jul 2024 07:21:30 -0700 Date: Mon, 22 Jul 2024 16:21:25 +0200 From: Blazej Kucman To: linux-pci@vger.kernel.org, ilpo.jarvinen@linux.intel.com, bhelgaas@google.com Cc: mstowe@redhat.com, mariusz.tkaczyk@linux.intel.com Subject: Re: [REGRESSION] Setting the LED status via attention stopped working. Message-ID: <20240722162125.00005e38@linux.intel.com> In-Reply-To: <20240719122253.00004b0e@linux.intel.com> References: <20240719122253.00004b0e@linux.intel.com> Organization: intel X-Mailer: Claws Mail 4.1.0 (GTK 3.24.33; x86_64-w64-mingw32) Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit On Fri, 19 Jul 2024 12:22:53 +0200 Blazej Kucman wrote: > Hi all, > > We discovered regression with setting LED state through "attention" > for VMD slot. e.g /sys/bus/pci/slots/2-1/attention. > > Expected behavior: > write status into "attention" will set the LED status correctly. > > Actual behavior: > write the status causes undefined behavior, "attention" contains a > different value than the one entered, the requested LED status is not > set > > After bisection kernel, it turned out that the issue appeared between > kernel v6.6..v6.7-rc1. Tested kernel: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git > > I found a patch that causes issue: > PCI: hotplug: Use FIELD_GET/PREP() > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v6.7-rc1&id=abaaac4845a0d6f39f83cbaba4c3b46ba5f93170 > I confirmed this by reverting this patch from kernel 6.10. > > Issue was discovered by using ledmon utility (ledctl is a part of > ledmon software) https://github.com/intel/ledmon, > example command: ledctl locate={/dev/nvme6n1 }, should set locate > status on nvme6n1 > > The values that ledmon writes into "attention" > > #define ATTENTION_OFF 0xF /* (1111) Attention Off, Power Off > */ #define ATTENTION_LOCATE 0x7 /* (0111) Attention Off, Power > On */ #define ATTENTION_REBUILD 0x5 /* (0101) Attention On, Power > On */ #define ATTENTION_FAILURE 0xD /* (1101) Attention On, Power > Off */ > > Without mentioned patch, writing these values will set the LEDs > correctly. > > I was able to determine that the change in behavior was caused by this > change @@ -484,7 +485,7 @@ int pciehp_set_raw_indicator_status(struct > hotplug_slot *hotplug_slot, struct pci_dev *pdev = ctrl_dev(ctrl); > > pci_config_pm_runtime_get(pdev); > - pcie_write_cmd_nowait(ctrl, status << 6, > + pcie_write_cmd_nowait(ctrl, FIELD_PREP(PCI_EXP_SLTCTL_AIC, > status), PCI_EXP_SLTCTL_AIC | PCI_EXP_SLTCTL_PIC); > pci_config_pm_runtime_put(pdev) > > I added logging of bit shift values and there is a significant > difference in the operation of this method > > int pciehp_set_raw_indicator_status(struct hotplug_slot *hotplug_slot, > u8 status) > { > struct controller *ctrl = to_ctrl(hotplug_slot); > struct pci_dev *pdev = ctrl_dev(ctrl); > > pr_err("SET_INDICATOR_ST START"); > > pr_err("SET_INDICATOR_ST_INPUT: STATUS: %d %u", status, > status); pr_err("SET_INDICATOR_ST_OLD d %d, u %u", status << 6, status > << 6); > > u8 test = FIELD_PREP(PCI_EXP_SLTCTL_AIC, status); > pr_err("SET_INDICATOR_ST_NEW d %d, u %u", test, test); > > pr_err("SET_INDICATOR_ST END"); > > pci_config_pm_runtime_get(pdev); > pcie_write_cmd_nowait(ctrl, status << 6, > PCI_EXP_SLTCTL_AIC | > PCI_EXP_SLTCTL_PIC); pci_config_pm_runtime_put(pdev); > return 0; > } > LOGS: > > [ 75.814893] SET_INDICATOR_ST START > [ 75.814895] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 75.818362] SET_INDICATOR_ST_OLD d 960, u 960 > [ 75.823143] SET_INDICATOR_ST_NEW d 192, u 192 > [ 75.827634] SET_INDICATOR_ST END > [ 75.832880] SET_INDICATOR_ST START > [ 75.836167] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 75.839626] SET_INDICATOR_ST_OLD d 960, u 960 > [ 75.844408] SET_INDICATOR_ST_NEW d 192, u 192 > [ 75.848834] SET_INDICATOR_ST END > [ 76.225732] SET_INDICATOR_ST START > [ 76.229022] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 76.232481] SET_INDICATOR_ST_OLD d 960, u 960 > [ 76.237259] SET_INDICATOR_ST_NEW d 192, u 192 > [ 76.241691] SET_INDICATOR_ST END > [ 76.250247] SET_INDICATOR_ST START > [ 76.253530] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 76.256990] SET_INDICATOR_ST_OLD d 960, u 960 > [ 76.261766] SET_INDICATOR_ST_NEW d 192, u 192 > [ 76.266189] SET_INDICATOR_ST END > [ 77.223562] SET_INDICATOR_ST START > [ 77.226852] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 77.230312] SET_INDICATOR_ST_OLD d 960, u 960 > [ 77.235091] SET_INDICATOR_ST_NEW d 192, u 192 > [ 77.239521] SET_INDICATOR_ST END > [ 77.244765] SET_INDICATOR_ST START > [ 77.248051] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 77.251510] SET_INDICATOR_ST_OLD d 960, u 960 > [ 77.256288] SET_INDICATOR_ST_NEW d 192, u 192 > [ 77.260725] SET_INDICATOR_ST END > [ 77.267606] SET_INDICATOR_ST START > > # ATTENTION_LOCATE > [ 77.270895] SET_INDICATOR_ST_INPUT: STATUS: 7 7 > [ 77.274356] SET_INDICATOR_ST_OLD d 448, u 448 > [ 77.278959] SET_INDICATOR_ST_NEW d 192, u 192 > [ 77.283379] SET_INDICATOR_ST END > > With patch PCI: hotplug: Use FIELD_GET/PREP(): > after trying to set ATTENTION_LOCATE, file "attention" contains value > 0x3 instead expected 0x7. > > [ 862.150910] SET_INDICATOR_ST START > [ 862.150912] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.154375] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.159159] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.163591] SET_INDICATOR_ST END > [ 862.169235] SET_INDICATOR_ST START > [ 862.172524] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.175984] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.182933] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.189190] SET_INDICATOR_ST END > [ 862.198302] SET_INDICATOR_ST START > [ 862.203290] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.208398] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.214780] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.220817] SET_INDICATOR_ST END > [ 862.231044] SET_INDICATOR_ST START > [ 862.235931] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.240994] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.247376] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.253411] SET_INDICATOR_ST END > [ 862.262574] SET_INDICATOR_ST START > [ 862.267471] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.272537] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.278918] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.284945] SET_INDICATOR_ST END > [ 862.291705] SET_INDICATOR_ST START > [ 862.296596] SET_INDICATOR_ST_INPUT: STATUS: 15 15 > [ 862.301656] SET_INDICATOR_ST_OLD d 960, u 960 > [ 862.308039] SET_INDICATOR_ST_NEW d 192, u 192 > [ 862.314064] SET_INDICATOR_ST END > > # ATTENTION_REBUILD > [ 862.322480] SET_INDICATOR_ST START > [ 862.327388] SET_INDICATOR_ST_INPUT: STATUS: 5 5 > [ 862.332454] SET_INDICATOR_ST_OLD d 320, u 320 > [ 862.338662] SET_INDICATOR_ST_NEW d 64, u 64 > [ 862.344704] SET_INDICATOR_ST END > > With patch PCI: hotplug: Use FIELD_GET/PREP(): > After trying to set ATTENTION_REBUILD, file "attention" contains value > 0x1 instead expected 0x5. > > This topic is quite important for us, because this change has been > included in RHEL9.5 kernel and causes LED setting for VMD drives to > not work. > > Looking at the above logs, I don't know if it always worked wrong, > because the input status is u8 and the bit shift did not cut off some > of higher bits e.g. for ATTENTION_LOCATE 0x7 (0111), > currently macro FIELD_PREP it does this through "AND" with mask. > > Regards, > Blazej > Hi, I sent a fix to mailing list. [PATCH] PCI: pciehp_hpc: Fix set raw indicator status https://lore.kernel.org/linux-pci/20240722141440.7210-1-blazej.kucman@intel.com/T/#u Thanks, Blazej