| 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
| ||
|
Message-ID: <a1a34c32-dbd4-7a77-ab7e-5e34af85900f@linux.intel.com>
Date: Tue, 18 Apr 2023 12:29:40 -0500
From: Dinh Nguyen <dinh.nguyen@...ux.intel.com>
To: Guenter Roeck <linux@...ck-us.net>, linux-hwmon@...r.kernel.org
Cc: dinguyen@...nel.org, devicetree@...r.kernel.org,
robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
linux-kernel@...r.kernel.org, jdelvare@...e.com,
Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
Subject: Re: [PATCH 3/5] hwmon: (socfpga) Add hardware monitoring support on
SoCFPGA platforms
On 4/17/2023 4:51 PM, Guenter Roeck wrote:
> On 4/17/23 13:55, Dinh Nguyen wrote:
>>
>> On 4/10/2023 9:44 PM, Guenter Roeck wrote:
>>> On 4/10/23 08:33, dinh.nguyen@...ux.intel.com wrote:
>>>> From: Dinh Nguyen <dinh.nguyen@...ux.intel.com>
>>>>
>>>> The driver supports 64-bit SoCFPGA platforms for temperature and
>>>> voltage
>>>> reading using the platform's SDM(Secure Device Manager). The driver
>>>> also uses the Stratix10 Service layer driver.
>>>>
>>>> This driver only supports OF SoCFPGA 64-bit platforms.
>>>>
>>>> Reviewed-by: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
>>>> Signed-off-by: Dinh Nguyen <dinh.nguyen@...ux.intel.com>
>>>> ---
>>>> Documentation/hwmon/index.rst | 1 +
>>>> Documentation/hwmon/socfpga-hwmon.rst | 30 ++
>>>> drivers/firmware/stratix10-svc.c | 18 +-
>>>
>>> Changes outside the hwmon subsystem need to be in a separate patch.
>>
>> will separate...
>>
>>>
>>>> drivers/hwmon/Kconfig | 11 +
>>>> drivers/hwmon/Makefile | 1 +
>>>> drivers/hwmon/socfpga-hwmon.c | 406
>>>> ++++++++++++++++++
>>>> include/linux/firmware/intel/stratix10-smc.h | 34 ++
>>>> .../firmware/intel/stratix10-svc-client.h | 6 +
>>>> 8 files changed, 506 insertions(+), 1 deletion(-)
>>>> create mode 100644 Documentation/hwmon/socfpga-hwmon.rst
>>>> create mode 100644 drivers/hwmon/socfpga-hwmon.c
>>>>
>>>>
>> ...
>>>> +
>>>> +enum hwmon_type_op {
>>>> + SOCFPGA_HWMON_TYPE_TEMP,
>>>> + SOCFPGA_HWMON_TYPE_VOLT,
>>>> + SOCFPGA_HWMON_TYPE_MAX
>>>
>>> Unused define
>>
>> Removed.
>>
>>>
>>>> +};
>>>> +
>>>> +static const char *const hwmon_types_str[] = { "temperature",
>>>> "voltage" };
>>>> +
>>>> +static umode_t socfpga_is_visible(const void *dev,
>>>> + enum hwmon_sensor_types type,
>>>> + u32 attr, int chan)
>>>> +{
>>>> + switch (type) {
>>>> + case hwmon_temp:
>>>> + case hwmon_in:
>>>> + return 0444;
>>>> + default:
>>>> + return 0;
>>>> + }
>>>> +}
>>>> +
>>>> +static void socfpga_smc_callback(struct stratix10_svc_client *client,
>>>> + struct stratix10_svc_cb_data *data)
>>>> +{
>>>> + struct socfpga_hwmon_priv *priv = client->priv;
>>>> + struct arm_smccc_res *res = data->kaddr1;
>>>> +
>>>> + if (data->status == BIT(SVC_STATUS_OK)) {
>>>> + if (priv->msg.command == COMMAND_HWMON_READTEMP)
>>>> + priv->temperature.value = res->a0;
>>>> + else
>>>> + priv->voltage.value = res->a0;
>>>> + } else
>>>> + dev_err(client->dev, "%s returned 0x%lX\n", __func__,
>>>> res->a0);
>>>> +
>>>
>>> Missing { } in else branch. Please run checkpatch --strict and fix
>>> continuation line alignment issues as well as unbalanced if/else
>>> reports.
>> Will do.
>>>
>>>> + complete(&priv->completion);
>>>> +}
>>>> +
>>>> +static int socfpga_hwmon_send(struct socfpga_hwmon_priv *priv)
>>>> +{
>>>> + int ret;
>>>> +
>>>> + priv->client.receive_cb = socfpga_smc_callback;
>>>> +
>>>> + ret = stratix10_svc_send(priv->chan, &priv->msg);
>>>> + if (ret < 0)
>>>> + return ret;
>>>> +
>>>> + if (!wait_for_completion_timeout(&priv->completion,
>>>> HWMON_TIMEOUT)) {
>>>> + dev_err(priv->client.dev, "SMC call timeout!\n");
>>>> + return -ETIMEDOUT;
>>>> + }
>>>> +
>>>> + return 0;
>>>> +}
>>>> +
>>>> +static int socfpga_hwmon_err_to_errno(struct socfpga_hwmon_priv
>>>> *priv)
>>>> +{
>>>> + int value = priv->temperature.value;
>>>> +
>>>> + if (!(value & ETEMP_ERROR))
>>>> + return 0;
>>>> +
>>>
>>> This is odd. int is normally 32 bit, this function is called from
>>> socfpga_read() for temperatures, which presumably are defined
>>> as "signed 32-bit fixed point binary". That means that negative
>>> temperatures would be treated as errors. Please verify.
>>
>> That's correct, if bit 31 is set, then it indicates an error.
>>
>
> This ...
>
>>>
>>>> + dev_err(priv->client.dev, "temperature sensor code 0x%08x\n",
>>>> value);
>>>> +
>>>
>>> Please don't clog the log with such messages.
>>
>> Removed.
>>
>>>
>>>> + value &= ~ETEMP_ERROR;
>>>> + switch (value) {
>>>> + case ETEMP_NOT_PRESENT:
>>>> + return -ENOENT;
>>>> + case ETEMP_CORRUPT:
>>>> + case ETEMP_NOT_INITIALIZED:
>>>> + return -ENODATA;
>>>> + case ETEMP_BUSY:
>>>> + return -EBUSY;
>>>> + case ETEMP_INACTIVE:
>>>> + case ETEMP_TIMEOUT:
>>>> + case ETEMP_TOO_OLD:
>>>> + return -EAGAIN;
>>>> + default:
>>>> + /* Unknown error */
>>>> + return -EINVAL;
>>>
>>> Should be -EIO.
>>>
>> Replaced.
>>>> + }
>>>> +}
>>>> +
>>>> +static int socfpga_read(struct device *dev, enum
>>>> hwmon_sensor_types type,
>>>> + u32 attr, int chan, long *val)
>>>> +{
>>>> + struct socfpga_hwmon_priv *priv = dev_get_drvdata(dev);
>>>> + int ret;
>>>> +
>>>> + mutex_lock(&priv->lock);
>>>> + reinit_completion(&priv->completion);
>>>> +
>>>> + switch (type) {
>>>> + case hwmon_temp:
>>>> + priv->msg.arg[0] = BIT_ULL(priv->temperature.chan[chan]);
>>>> + priv->msg.command = COMMAND_HWMON_READTEMP;
>>>> + if (socfpga_hwmon_send(priv))
>>>> + goto status_done;
>>>> +
>>>> + ret = socfpga_hwmon_err_to_errno(priv);
>>>> + if (ret)
>>>> + break;
>>>> + /*
>>>> + * The Temperature Sensor IP core returns the Celsius
>>>> + * temperature value in signed 32-bit fixed point binary
>
> ... and this contradict each other. If bit 31 indicates an error,
> this can not be a signed 32-bit value.
>
You're right! I've re-read the spec and should have the the code look
for the specific error values:
0x80000000 - inactive
0x80000001 - old value
0x80000002 - invalid channel
0x80000003 - corrupted.
...
Dinh
Powered by blists - more mailing lists