On Tuesday 23 November 2021 08:00:51 Guenter Roeck wrote: > On Mon, Nov 22, 2021 at 08:43:10PM +0100, Pali Rohár wrote: > > On Monday 22 November 2021 11:28:30 Guenter Roeck wrote: > > > On 11/22/21 11:10 AM, Pali Rohár wrote: > > > > On Monday 22 November 2021 19:50:14 Armin Wolf wrote: > > > > > Am 22.11.21 um 18:55 schrieb Guenter Roeck: > > > > > > On 11/22/21 8:01 AM, Pali Rohár wrote: > > > > > > > On Saturday 20 November 2021 18:03:19 Armin Wolf wrote: > > > > > > > > The only purpose of i8k_ioctl() is to call i8k_ioctl_unlocked() > > > > > > > > with i8k_mutex held. Judging from the hwmon code, this mutex > > > > > > > > only needs to be held when setting the fan speed/mode. > > > > > > > > > > > > > > Really? I think that there is no difference between setting and getting > > > > > > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do > > > > > > > not need it. Some more explanation is needed... > > > > > > > > > > > > > I8K_SET_FAN sets the fan speed and returns the current status. Without > > > > > > locking, the returned status may not match or be associated with the > > > > > > previous > > > > > > set operation. > > > > > > > > > > > > Maybe that doesn't matter, and the synchronization is not needed. If so, > > > > > > you can probably remove the locking entirely. > > > > > > > > > > > > Guenter > > > > > > > > > > That is the reason i kept the locking code. Since i do not want to break > > > > > the ioctl interfacein any way, removing the locking code seems too risky > > > > > to me. > > > > > > > > I see. That is a good point. > > > > > > > > But there is same race condition also when at the same time going to > > > > change speed via ioctl and also via hwmon sysfs. > > > > > > > > > > I thought the sysfs code does not change the fan speed and report the > > > fan status in the same request. Did I miss something ? > > > > No. I mean something different. Let me to write trace call: > > > > CPU 0: CPU 1: > > 1. dell_smm_write() > > 1. ioctl(I8K_SET_FAN) > > 2. i8k_set_fan() > > 2. i8k_set_fan() > > 3. i8k_get_fan_status() > > > > So to ensure that i8k_get_fan_status() on CPU 0 returns value which > > belongs to i8k_set_fan() from CPU 0 it is needed to still use mutex. > > > > Armin is right here and I think that patch is correct. > > Quoting your earlier reply: > > > Really? I think that there is no difference between setting and getting > > fan speed/mode. At least I do not see why 'set' needs mutex and 'get' do > > not need it. Some more explanation is needed... > > This was the reason for my comment. Your latest reply is leaving me a bit > puzzled. If you are ok with the patch as-is, please provide a Reviewed-by: > or Acked-by: tag. Sorry for that. I should have explicitly wrote that I realized how it works after Armin explained it.
