From: Quan Nguyen <quan@os.amperecomputing.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: Lee Jones <lee.jones@linaro.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Jean Delvare <jdelvare@suse.com>, Guenter Roeck <linux@roeck-us.net>, Jonathan Corbet <corbet@lwn.net>, Derek Kiernan <derek.kiernan@xilinx.com>, Dragan Cvetic <dragan.cvetic@xilinx.com>, Arnd Bergmann <arnd@arndb.de>, Thu Nguyen <thu@os.amperecomputing.com>, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, OpenBMC Maillist <openbmc@lists.ozlabs.org>, Open Source Submission <patches@amperecomputing.com>, Phong Vo <phong@os.amperecomputing.com>, "Thang Q . Nguyen" <thang@os.amperecomputing.com> Subject: Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver Date: Thu, 2 Jun 2022 16:36:22 +0700 [thread overview] Message-ID: <524c6acf-06a8-6e63-c9be-0bbf730fa693@os.amperecomputing.com> (raw) In-Reply-To: <YpcyaTqqsfDJx7HG@kroah.com> On 01/06/2022 16:33, Greg Kroah-Hartman wrote: > On Wed, Jun 01, 2022 at 03:21:47PM +0700, Quan Nguyen wrote: >>>> + if (err_type & BIT(2)) { >>>> + /* Error with data type */ >>>> + ret = regmap_read(errmon->regmap, err_info->err_data_low, &data_lo); >>>> + if (ret) >>>> + goto done; >>>> + >>>> + ret = regmap_read(errmon->regmap, err_info->err_data_high, &data_hi); >>>> + if (ret) >>>> + goto done; >>>> + >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 4, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >>>> + /* clear the read errors */ >>>> + ret = regmap_write(errmon->regmap, err_info->err_type, BIT(2)); >>>> + >>>> + } else if (err_type & BIT(1)) { >>>> + /* Error type */ >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 2, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >>>> + /* clear the read errors */ >>>> + ret = regmap_write(errmon->regmap, err_info->err_type, BIT(1)); >>>> + >>>> + } else if (err_type & BIT(0)) { >>>> + /* Warning type */ >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 1, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >> >> Hi Greg, >> >> Since the internal representation of the internal error is split into high >> low chunks of the info and data values which need to be communicated >> atomicly, I'm treating them as "one value" here. > > That is a huge "one value", that's not what this really is, it needs to > be parsed by userspace, right? > Thanks Greg for the review, User space needs all of this "one value" to know what exactly is the error. In our latest version, we remove all the if...else and simplify the code as below: /* * The internal representation of the internal error is split into high * low chunks of the info and data values. Rather than temporarily * dumping these into an array and printing that, skip the intermediate * step and print them using a concatenation encoding. */ count = sysfs_emit(buf, "%04x%04x%04x%04x\n", info_h, info_l, data_h, data_l); /* clear the read error */ ret = regmap_write(errmon->regmap, err_info->type, err_type); return ret ? ret : count; > And why does this have to be atomic? What happens if the values change > right after you read them? What is userspace going to do with them? > Because the error is bigger than single register can hold so it is split into small chunks to report via multiple separate registers. Firmware stores each error in a queue. As the error's chunks are stored in separate registers. All of these registers will need to be read out before the error is clear so that the next error in the queue can be reported. That is why we say those chunks must be read out atomically. User space will need to parse these information themself. >> I could dump them in a >> temporary array and print that, but it seems like additional complexity for >> the same result. Can we consider this concatenated encoding as "an array of >> the same type" for the purposes of this driver?" > > That's really not a good idea as sysfs files should never need to be > "parsed" like this. > > Again, what are you trying to do here, and why does it have to be > atomic? > > thanks, > > greg k-h
WARNING: multiple messages have this Message-ID (diff)
From: Quan Nguyen <quan@os.amperecomputing.com> To: Greg Kroah-Hartman <gregkh@linuxfoundation.org> Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, Jean Delvare <jdelvare@suse.com>, Phong Vo <phong@os.amperecomputing.com>, Arnd Bergmann <arnd@arndb.de>, Jonathan Corbet <corbet@lwn.net>, Dragan Cvetic <dragan.cvetic@xilinx.com>, linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, "Thang Q . Nguyen" <thang@os.amperecomputing.com>, OpenBMC Maillist <openbmc@lists.ozlabs.org>, Rob Herring <robh+dt@kernel.org>, Krzysztof Kozlowski <krzysztof.kozlowski+dt@linaro.org>, Derek Kiernan <derek.kiernan@xilinx.com>, Open Source Submission <patches@amperecomputing.com>, Lee Jones <lee.jones@linaro.org>, Thu Nguyen <thu@os.amperecomputing.com>, Guenter Roeck <linux@roeck-us.net> Subject: Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver Date: Thu, 2 Jun 2022 16:36:22 +0700 [thread overview] Message-ID: <524c6acf-06a8-6e63-c9be-0bbf730fa693@os.amperecomputing.com> (raw) In-Reply-To: <YpcyaTqqsfDJx7HG@kroah.com> On 01/06/2022 16:33, Greg Kroah-Hartman wrote: > On Wed, Jun 01, 2022 at 03:21:47PM +0700, Quan Nguyen wrote: >>>> + if (err_type & BIT(2)) { >>>> + /* Error with data type */ >>>> + ret = regmap_read(errmon->regmap, err_info->err_data_low, &data_lo); >>>> + if (ret) >>>> + goto done; >>>> + >>>> + ret = regmap_read(errmon->regmap, err_info->err_data_high, &data_hi); >>>> + if (ret) >>>> + goto done; >>>> + >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 4, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >>>> + /* clear the read errors */ >>>> + ret = regmap_write(errmon->regmap, err_info->err_type, BIT(2)); >>>> + >>>> + } else if (err_type & BIT(1)) { >>>> + /* Error type */ >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 2, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >>>> + /* clear the read errors */ >>>> + ret = regmap_write(errmon->regmap, err_info->err_type, BIT(1)); >>>> + >>>> + } else if (err_type & BIT(0)) { >>>> + /* Warning type */ >>>> + count = sysfs_emit(buf, "%01x%02x%01x%02x%04x%04x%04x\n", >>>> + 1, (ret_hi & 0xf000) >> 12, (ret_hi & 0x0800) >> 11, >>>> + ret_hi & 0xff, ret_lo, data_hi, data_lo); >> >> Hi Greg, >> >> Since the internal representation of the internal error is split into high >> low chunks of the info and data values which need to be communicated >> atomicly, I'm treating them as "one value" here. > > That is a huge "one value", that's not what this really is, it needs to > be parsed by userspace, right? > Thanks Greg for the review, User space needs all of this "one value" to know what exactly is the error. In our latest version, we remove all the if...else and simplify the code as below: /* * The internal representation of the internal error is split into high * low chunks of the info and data values. Rather than temporarily * dumping these into an array and printing that, skip the intermediate * step and print them using a concatenation encoding. */ count = sysfs_emit(buf, "%04x%04x%04x%04x\n", info_h, info_l, data_h, data_l); /* clear the read error */ ret = regmap_write(errmon->regmap, err_info->type, err_type); return ret ? ret : count; > And why does this have to be atomic? What happens if the values change > right after you read them? What is userspace going to do with them? > Because the error is bigger than single register can hold so it is split into small chunks to report via multiple separate registers. Firmware stores each error in a queue. As the error's chunks are stored in separate registers. All of these registers will need to be read out before the error is clear so that the next error in the queue can be reported. That is why we say those chunks must be read out atomically. User space will need to parse these information themself. >> I could dump them in a >> temporary array and print that, but it seems like additional complexity for >> the same result. Can we consider this concatenated encoding as "an array of >> the same type" for the purposes of this driver?" > > That's really not a good idea as sysfs files should never need to be > "parsed" like this. > > Again, what are you trying to do here, and why does it have to be > atomic? > > thanks, > > greg k-h
next prev parent reply other threads:[~2022-06-02 9:36 UTC|newest] Thread overview: 44+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-04-22 2:46 [PATCH v8 0/9] Add Ampere's Altra SMPro MFD and its child drivers Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 1/9] hwmon: smpro: Add Ampere's Altra smpro-hwmon driver Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 2/9] docs: hwmon: (smpro-hwmon) Add documentation Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 6:20 ` Greg Kroah-Hartman 2022-04-22 6:20 ` Greg Kroah-Hartman 2022-04-22 14:43 ` Quan Nguyen 2022-04-22 14:43 ` Quan Nguyen 2022-04-22 14:57 ` Greg Kroah-Hartman 2022-04-22 14:57 ` Greg Kroah-Hartman 2022-04-22 17:07 ` Darren Hart 2022-04-22 17:07 ` Darren Hart 2022-04-22 23:21 ` Quan Nguyen 2022-04-22 23:21 ` Quan Nguyen 2022-06-01 8:21 ` Quan Nguyen 2022-06-01 8:21 ` Quan Nguyen 2022-06-01 9:33 ` Greg Kroah-Hartman 2022-06-01 9:33 ` Greg Kroah-Hartman 2022-06-02 9:36 ` Quan Nguyen [this message] 2022-06-02 9:36 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 4/9] docs: misc-devices: (smpro-errmon) Add documentation Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 6:27 ` Bagas Sanjaya 2022-04-22 6:27 ` Bagas Sanjaya 2022-04-22 2:46 ` [PATCH v8 5/9] misc: smpro-misc: Add Ampere's Altra SMpro misc driver Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 6/9] docs: misc-devices: (smpro-misc) Add documentation Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 7/9] dt-bindings: mfd: Add bindings for Ampere Altra SMPro MFD driver Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 8/9] mfd: smpro-mfd: Adds Ampere's Altra SMpro " Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen 2022-04-22 21:19 ` kernel test robot 2022-04-22 21:19 ` kernel test robot 2022-06-15 22:14 ` Lee Jones 2022-06-15 22:14 ` Lee Jones 2022-06-29 9:23 ` Quan Nguyen 2022-06-29 9:23 ` Quan Nguyen 2022-04-22 2:46 ` [PATCH v8 9/9] docs: ABI: testing: Document the Ampere Altra Family's SMpro sysfs interfaces Quan Nguyen 2022-04-22 2:46 ` Quan Nguyen
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=524c6acf-06a8-6e63-c9be-0bbf730fa693@os.amperecomputing.com \ --to=quan@os.amperecomputing.com \ --cc=arnd@arndb.de \ --cc=corbet@lwn.net \ --cc=derek.kiernan@xilinx.com \ --cc=devicetree@vger.kernel.org \ --cc=dragan.cvetic@xilinx.com \ --cc=gregkh@linuxfoundation.org \ --cc=jdelvare@suse.com \ --cc=krzysztof.kozlowski+dt@linaro.org \ --cc=lee.jones@linaro.org \ --cc=linux-doc@vger.kernel.org \ --cc=linux-hwmon@vger.kernel.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux@roeck-us.net \ --cc=openbmc@lists.ozlabs.org \ --cc=patches@amperecomputing.com \ --cc=phong@os.amperecomputing.com \ --cc=robh+dt@kernel.org \ --cc=thang@os.amperecomputing.com \ --cc=thu@os.amperecomputing.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.