From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from mgamail.intel.com (mgamail.intel.com [192.198.163.16]) (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 EF0CA1F1302; Wed, 21 May 2025 11:06:10 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=192.198.163.16 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1747825572; cv=none; b=qgxEms9Y52LYDOj/Z/Zh625g7S4JT27R4HFIYHf+RHkNgpZyvHNMvyLqCm6stVIPb8S0985o6GpzbB2nIqtrEsb2fmLQ6uL+IP0xMa75Ox5YK0u01h1rfa/A9GylehVkJXjXFciDRGypeSzScWXGytGUhGPBTEK8lFf1b9M0yoo= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1747825572; c=relaxed/simple; bh=utfKUeejFLvJbdLWTRYs6J3vHpGRVnI8RSEnIEARCWQ=; h=From:Date:To:cc:Subject:In-Reply-To:Message-ID:References: MIME-Version:Content-Type; b=LCMAoBiORUfn2WctEY9XjMOem1S7VKKBBDfNIGtG7nsV+snHLQJ/7zayjMEkH17XlVirtfvOZrhuUTwg2uX90mx4xyY8+4PCG0YDCQhspeWGkxPBhOKQAhHTaxBRQjR6RzPMh1qgHXN5EIrr/ltyLemYxAzUxZXxfPLOEBIXWBU= 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=Z1RXqq0U; arc=none smtp.client-ip=192.198.163.16 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="Z1RXqq0U" DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=intel.com; i=@intel.com; q=dns/txt; s=Intel; t=1747825571; x=1779361571; h=from:date:to:cc:subject:in-reply-to:message-id: references:mime-version; bh=utfKUeejFLvJbdLWTRYs6J3vHpGRVnI8RSEnIEARCWQ=; b=Z1RXqq0UhPhdgiUxP0BNTjtBHmTSYUf8Bp13CbtiLpsDXcDW0zwX02PK Wzy5imt99Y4MuWd6cakcATmRCTP8E2AS0Q1TQfceJJzgKPM3cjYWpwKRT SE5vzNHiNO0pW8shtDEV11KZ6ZL+uAfvudbwgdh/XocfbhpFdyx93Y75g CsnrKgdlWBy7FEqH9w9wO0nIG/u6mHBR8gADtlMFZLw9dMGMatn6vIS9D mKwP99JxHEXTcAJP07xiF0yW84XMw4/2fz/EescIqhldpwcNNZ1oZz8SR joIxI9EfClJio5vskkQKFF/9YBRlbwYK2GFxY2FPqfd9VA5ohq/EtZgqn w==; X-CSE-ConnectionGUID: z014iHJSRuCxgJszyDNlbQ== X-CSE-MsgGUID: dggh41SETXSjD7W+jL+Vlw== X-IronPort-AV: E=McAfee;i="6700,10204,11439"; a="37418474" X-IronPort-AV: E=Sophos;i="6.15,303,1739865600"; d="scan'208";a="37418474" Received: from orviesa007.jf.intel.com ([10.64.159.147]) by fmvoesa110.fm.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 May 2025 04:06:10 -0700 X-CSE-ConnectionGUID: dafv0W/nR22u+DF84vjbWQ== X-CSE-MsgGUID: DqV9Ekb3RyGrP/3IuGZTHw== X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="6.15,303,1739865600"; d="scan'208";a="140529464" Received: from dalessan-mobl3.ger.corp.intel.com (HELO localhost) ([10.245.244.221]) by orviesa007-auth.jf.intel.com with ESMTP/TLS/ECDHE-RSA-AES256-GCM-SHA384; 21 May 2025 04:06:01 -0700 From: =?UTF-8?q?Ilpo=20J=C3=A4rvinen?= Date: Wed, 21 May 2025 14:05:57 +0300 (EEST) To: Jonathan Cameron cc: Bjorn Helgaas , linux-pci@vger.kernel.org, Jon Pan-Doh , Karolina Stolarek , Weinan Liu , Martin Petersen , Ben Fuller , Drew Walton , Anil Agrawal , Tony Luck , Sathyanarayanan Kuppuswamy , Lukas Wunner , Sargun Dhillon , "Paul E . McKenney" , Mahesh J Salgaonkar , Oliver O'Halloran , Kai-Heng Feng , Keith Busch , Robert Richter , Terry Bowman , Shiju Jose , Dave Jiang , LKML , linuxppc-dev@lists.ozlabs.org, Bjorn Helgaas , =?ISO-8859-2?Q?Krzysztof_Wilczy=F1ski?= Subject: Re: [PATCH v7 17/17] PCI/AER: Add sysfs attributes for log ratelimits In-Reply-To: <20250521114600.00007010@huawei.com> Message-ID: <5f9fee34-1db6-d30b-688f-040570cc651a@linux.intel.com> References: <20250520215047.1350603-1-helgaas@kernel.org> <20250520215047.1350603-18-helgaas@kernel.org> <20250521114600.00007010@huawei.com> Precedence: bulk X-Mailing-List: linux-pci@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="8323328-1454175955-1747825557=:946" This message is in MIME format. The first part should be readable text, while the remaining parts are likely unreadable without MIME-aware tools. --8323328-1454175955-1747825557=:946 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: QUOTED-PRINTABLE On Wed, 21 May 2025, Jonathan Cameron wrote: > On Tue, 20 May 2025 16:50:34 -0500 > Bjorn Helgaas wrote: >=20 > > From: Jon Pan-Doh > >=20 > > Allow userspace to read/write log ratelimits per device (including > > enable/disable). Create aer/ sysfs directory to store them and any > > future aer configs. > >=20 > > Update AER sysfs ABI filename to reflect the broader scope of AER sysfs > > attributes (e.g. stats and ratelimits). > >=20 > > Documentation/ABI/testing/sysfs-bus-pci-devices-aer_stats -> > > sysfs-bus-pci-devices-aer > >=20 > > Tested using aer-inject[1]. Configured correctable log ratelimit to 5. > > Sent 6 AER errors. Observed 5 errors logged while AER stats > > (cat /sys/bus/pci/devices//aer_dev_correctable) shows 6. > >=20 > > Disabled ratelimiting and sent 6 more AER errors. Observed all 6 errors > > logged and accounted in AER stats (12 total errors). > >=20 > > [1] https://git.kernel.org/pub/scm/linux/kernel/git/gong.chen/aer-injec= t.git > >=20 > > [bhelgaas: note fatal errors are not ratelimited, "aer_report" -> "aer_= info"] > > Signed-off-by: Karolina Stolarek > > Signed-off-by: Jon Pan-Doh > > Signed-off-by: Bjorn Helgaas > > Tested-by: Krzysztof Wilczy=C5=84ski >=20 > There is some relatively new SYSFS infra that I think will help > make this slightly nicer by getting rid of the extra directory when > there is nothing to be done with it. >=20 > > --- > > ...es-aer_stats =3D> sysfs-bus-pci-devices-aer} | 34 +++++++ > > Documentation/PCI/pcieaer-howto.rst | 5 +- > > drivers/pci/pci-sysfs.c | 1 + > > drivers/pci/pci.h | 1 + > > drivers/pci/pcie/aer.c | 99 +++++++++++++++++++ > > 5 files changed, 139 insertions(+), 1 deletion(-) > > rename Documentation/ABI/testing/{sysfs-bus-pci-devices-aer_stats =3D>= sysfs-bus-pci-devices-aer} (77%) >=20 >=20 > > diff --git a/drivers/pci/pcie/aer.c b/drivers/pci/pcie/aer.c > > index f9e684ac7878..9b8dea317a79 100644 > > --- a/drivers/pci/pcie/aer.c > > +++ b/drivers/pci/pcie/aer.c > > @@ -627,6 +627,105 @@ const struct attribute_group aer_stats_attr_group= =3D { > > =09.is_visible =3D aer_stats_attrs_are_visible, > > }; >=20 > > +#define aer_ratelimit_burst_attr(name, ratelimit)=09=09=09\ > > +=09static ssize_t=09=09=09=09=09=09=09\ > > +=09name##_show(struct device *dev, struct device_attribute *attr,=09\ > > +=09=09 char *buf)=09=09=09=09=09=09\ > > +{=09=09=09=09=09=09=09=09=09\ >=20 > A little odd looking to indent this less than the line above. >=20 > > +=09struct pci_dev *pdev =3D to_pci_dev(dev);=09=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09return sysfs_emit(buf, "%d\n",=09=09=09=09=09\ > > +=09=09=09 pdev->aer_info->ratelimit.burst);=09=09\ > > +}=09=09=09=09=09=09=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09static ssize_t=09=09=09=09=09=09=09\ > > +=09name##_store(struct device *dev, struct device_attribute *attr,=09\ > > +=09=09 const char *buf, size_t count)=09=09=09\ > > +{=09=09=09=09=09=09=09=09=09\ > > +=09struct pci_dev *pdev =3D to_pci_dev(dev);=09=09=09=09\ > > +=09int burst;=09=09=09=09=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09if (!capable(CAP_SYS_ADMIN))=09=09=09=09=09\ > > +=09=09return -EPERM;=09=09=09=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09if (kstrtoint(buf, 0, &burst) < 0)=09=09=09=09\ > > +=09=09return -EINVAL;=09=09=09=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09pdev->aer_info->ratelimit.burst =3D burst;=09=09=09\ > > +=09=09=09=09=09=09=09=09=09\ > > +=09return count;=09=09=09=09=09=09=09\ > > +}=09=09=09=09=09=09=09=09=09\ > > +static DEVICE_ATTR_RW(name) > > + > > +aer_ratelimit_burst_attr(ratelimit_burst_cor_log, cor_log_ratelimit); > > +aer_ratelimit_burst_attr(ratelimit_burst_uncor_log, uncor_log_ratelimi= t); > > + > > +static struct attribute *aer_attrs[] =3D { > > +=09&dev_attr_ratelimit_log_enable.attr, > > +=09&dev_attr_ratelimit_burst_cor_log.attr, > > +=09&dev_attr_ratelimit_burst_uncor_log.attr, > > +=09NULL > > +}; > > + > > +static umode_t aer_attrs_are_visible(struct kobject *kobj, > > +=09=09=09=09 struct attribute *a, int n) > > +{ > > +=09struct device *dev =3D kobj_to_dev(kobj); > > +=09struct pci_dev *pdev =3D to_pci_dev(dev); > > + > > +=09if (!pdev->aer_info) > > +=09=09return 0; > > + > > +=09return a->mode; > > +} > > + > > +const struct attribute_group aer_attr_group =3D { > > +=09.name =3D "aer", > > +=09.attrs =3D aer_attrs, > > +=09.is_visible =3D aer_attrs_are_visible, > > +}; >=20 > There are a bunch of macros to simplify cases where > a whole group is either enabled or not and make the group > itself go away if there is nothing to be shown. >=20 > DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE() combined with > SYSFS_GROUP_VISIBLE() around the assignment does what we > want here I think. >=20 > Whilst we can't retrofit that stuff onto existing ABI > as someone may be assuming directory presence, Are you sure about this? That empty directories are part of ABI as well? Are any of these directories listed under Documentation/ABI ? I can see somebody could in theory rely on the existance of empty=20 directories but it's not like it contains any real substance without a file with the actual content of interest so it seems somewhat strange to check for directory and not the file of interest itself. > we can make sysfs less cluttered for new stuff. >=20 > Maybe I'm missing why that doesn't work here though! >=20 > J >=20 > > + > > static void pci_dev_aer_stats_incr(struct pci_dev *pdev, > > =09=09=09=09 struct aer_err_info *info) > > { >=20 --=20 i. --8323328-1454175955-1747825557=:946--