linux-kernel - Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 1 Jun 2022 11:33:29 +0200
From:   Greg Kroah-Hartman <gregkh@...uxfoundation.org>
To:     Quan Nguyen <quan@...amperecomputing.com>
Cc:     Lee Jones <lee.jones@...aro.org>, Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        Jean Delvare <jdelvare@...e.com>,
        Guenter Roeck <linux@...ck-us.net>,
        Jonathan Corbet <corbet@....net>,
        Derek Kiernan <derek.kiernan@...inx.com>,
        Dragan Cvetic <dragan.cvetic@...inx.com>,
        Arnd Bergmann <arnd@...db.de>,
        Thu Nguyen <thu@...amperecomputing.com>,
        linux-kernel@...r.kernel.org, devicetree@...r.kernel.org,
        linux-hwmon@...r.kernel.org, linux-doc@...r.kernel.org,
        OpenBMC Maillist <openbmc@...ts.ozlabs.org>,
        Open Source Submission <patches@...erecomputing.com>,
        Phong Vo <phong@...amperecomputing.com>,
        "Thang Q . Nguyen" <thang@...amperecomputing.com>
Subject: Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error
 monitor driver

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?

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?

> 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