Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210304105830.507176-1-kubernat@cesnet.cz> <20210304105830.507176-3-kubernat@cesnet.cz>
 <ad1c3054-e9a2-4299-aeed-94e36d6d3d12@cesnet.cz>
In-Reply-To: <ad1c3054-e9a2-4299-aeed-94e36d6d3d12@cesnet.cz>
From:   =?UTF-8?B?VsOhY2xhdiBLdWJlcm7DoXQ=?= <kubernat@cesnet.cz>
Date:   Mon, 8 Mar 2021 10:31:39 +0100
Message-ID: <CABKa3noLPxg8DFoHBG7nz5h12Bo0qTU7sWbJBzsgtvK69aB0uw@mail.gmail.com>
Subject: Re: [PATCH 3/7] hwmon: (max31790) Allow setting pulses
To:     =?UTF-8?B?SmFuIEt1bmRyw6F0?= <jan.kundrat@cesnet.cz>
Cc:     Jean Delvare <jdelvare@suse.com>,
        Guenter Roeck <linux@roeck-us.net>,
        Jonathan Corbet <corbet@lwn.net>, linux-hwmon@vger.kernel.org,
        linux-doc@vger.kernel.org, linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

Thanks, I will include fixes in v2 of the patches.

By the way, I'd like to mention that Jan is my colleague.

V=C3=A1clav

p=C3=A1 5. 3. 2021 v 13:08 odes=C3=ADlatel Jan Kundr=C3=A1t <jan.kundrat@ce=
snet.cz> napsal:
>
> > @@ -285,6 +295,9 @@ static int max31790_write_fan(struct device
> > *dev, u32 attr, int channel,
> >                                  MAX31790_REG_FAN_CONFIG(channel),
> >                                  data->fan_config[channel]);
> >               break;
> > +     case hwmon_fan_pulses:
> > +             data->pulses[channel] =3D val;
> > +             break;
>
> This needs input validation, otherwise it's possible to write 0 in there
> and you get a division-by-zero in the kernel context:
>
> [102109.999968] Division by zero in kernel.
> [102110.003917] CPU: 1 PID: 27590 Comm: cat Not tainted 5.9.3-cla-cfb #42
> [102110.010462] Hardware name: Marvell Armada 380/385 (Device Tree)
> [102110.016497] [<c010f16c>] (unwind_backtrace) from [<c010ae40>]
> (show_stack+0x10/0x14)
> [102110.024355] [<c010ae40>] (show_stack) from [<c083ba30>]
> (dump_stack+0x94/0xa8)
> [102110.031689] [<c083ba30>] (dump_stack) from [<c083a3fc>]
> (Ldiv0+0x8/0x2c)
> [102110.038499] [<c083a3fc>] (Ldiv0) from [<c064c1ac>]
> (max31790_read+0x174/0x204)
> [102110.045836] [<c064c1ac>] (max31790_read) from [<c0646fdc>]
> (hwmon_attr_show+0x44/0x138)
> ...
>
> A similar error can also happen when setting the fan speed to 0 RPM.
> That's, however, not an error caused by this patch series AFAIK. I *think=
*
> that RPM_TO_REG should be changed to check if `rpm` is 0, and if so, set
> the register directly to the maximal value of 0x7ff (in another patch).
>
> With kind regards,
> Jan