From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 08A99C433F5 for ; Wed, 1 Jun 2022 09:33:56 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1351117AbiFAJdy (ORCPT ); Wed, 1 Jun 2022 05:33:54 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:49292 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1351127AbiFAJdx (ORCPT ); Wed, 1 Jun 2022 05:33:53 -0400 Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 786258B09E; Wed, 1 Jun 2022 02:33:52 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 0E3FBB8185E; Wed, 1 Jun 2022 09:33:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4F7B4C385A5; Wed, 1 Jun 2022 09:33:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1654076029; bh=5T6DOvFw902lROs1sSAyrJ/DZ8vBhIwjqturHxstVd8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bGGzdgsl0yWRtC4tWhYShHmsSXAkbeeoS1eRaWgAuHlUB8ydG6LiQgI9ENIJpxdvi lI2Zh0dsTLjglJKNtjG3AJsMHdauUUCWmZMnJDlDaUW6FNuR9a/bnbZ2OsY9rYdlzs 1s/mzxOUsVKKevlFm5kdHfOHd2SOWfgorHG4Rtuw= Date: Wed, 1 Jun 2022 11:33:29 +0200 From: Greg Kroah-Hartman To: Quan Nguyen Cc: Lee Jones , Rob Herring , Krzysztof Kozlowski , Jean Delvare , Guenter Roeck , Jonathan Corbet , Derek Kiernan , Dragan Cvetic , Arnd Bergmann , Thu Nguyen , linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linux-hwmon@vger.kernel.org, linux-doc@vger.kernel.org, OpenBMC Maillist , Open Source Submission , Phong Vo , "Thang Q . Nguyen" Subject: Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver Message-ID: References: <20220422024653.2199489-1-quan@os.amperecomputing.com> <20220422024653.2199489-4-quan@os.amperecomputing.com> <4f5d7746-3747-4a4d-525a-4fb69e706cd0@os.amperecomputing.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4f5d7746-3747-4a4d-525a-4fb69e706cd0@os.amperecomputing.com> Precedence: bulk List-ID: X-Mailing-List: linux-hwmon@vger.kernel.org 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 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from lists.ozlabs.org (lists.ozlabs.org [112.213.38.117]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id C5564C433EF for ; Wed, 1 Jun 2022 09:34:35 +0000 (UTC) Received: from boromir.ozlabs.org (localhost [IPv6:::1]) by lists.ozlabs.org (Postfix) with ESMTP id 4LCkWG1RFpz2xXw for ; Wed, 1 Jun 2022 19:34:34 +1000 (AEST) Authentication-Results: lists.ozlabs.org; dkim=fail reason="signature verification failed" (1024-bit key; unprotected) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.a=rsa-sha256 header.s=korg header.b=bGGzdgsl; dkim-atps=neutral Authentication-Results: lists.ozlabs.org; spf=pass (sender SPF authorized) smtp.mailfrom=linuxfoundation.org (client-ip=145.40.68.75; helo=ams.source.kernel.org; envelope-from=gregkh@linuxfoundation.org; receiver=) Authentication-Results: lists.ozlabs.org; dkim=pass (1024-bit key; unprotected) header.d=linuxfoundation.org header.i=@linuxfoundation.org header.a=rsa-sha256 header.s=korg header.b=bGGzdgsl; dkim-atps=neutral Received: from ams.source.kernel.org (ams.source.kernel.org [145.40.68.75]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by lists.ozlabs.org (Postfix) with ESMTPS id 4LCkVW6FYWz3bYG for ; Wed, 1 Jun 2022 19:33:54 +1000 (AEST) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ams.source.kernel.org (Postfix) with ESMTPS id 021CFB8185D; Wed, 1 Jun 2022 09:33:51 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 4F7B4C385A5; Wed, 1 Jun 2022 09:33:47 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1654076029; bh=5T6DOvFw902lROs1sSAyrJ/DZ8vBhIwjqturHxstVd8=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=bGGzdgsl0yWRtC4tWhYShHmsSXAkbeeoS1eRaWgAuHlUB8ydG6LiQgI9ENIJpxdvi lI2Zh0dsTLjglJKNtjG3AJsMHdauUUCWmZMnJDlDaUW6FNuR9a/bnbZ2OsY9rYdlzs 1s/mzxOUsVKKevlFm5kdHfOHd2SOWfgorHG4Rtuw= Date: Wed, 1 Jun 2022 11:33:29 +0200 From: Greg Kroah-Hartman To: Quan Nguyen Subject: Re: [PATCH v8 3/9] misc: smpro-errmon: Add Ampere's SMpro error monitor driver Message-ID: References: <20220422024653.2199489-1-quan@os.amperecomputing.com> <20220422024653.2199489-4-quan@os.amperecomputing.com> <4f5d7746-3747-4a4d-525a-4fb69e706cd0@os.amperecomputing.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <4f5d7746-3747-4a4d-525a-4fb69e706cd0@os.amperecomputing.com> X-BeenThere: openbmc@lists.ozlabs.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Development list for OpenBMC List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: linux-hwmon@vger.kernel.org, devicetree@vger.kernel.org, Jean Delvare , Phong Vo , Arnd Bergmann , Jonathan Corbet , Dragan Cvetic , linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org, "Thang Q . Nguyen" , OpenBMC Maillist , Rob Herring , Krzysztof Kozlowski , Derek Kiernan , Open Source Submission , Lee Jones , Thu Nguyen , Guenter Roeck Errors-To: openbmc-bounces+openbmc=archiver.kernel.org@lists.ozlabs.org Sender: "openbmc" 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