> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar@xxxxxxxxx>
> Sent: Wednesday, September 27, 2023 2:35 PM
> To: Dixit, Ashutosh <ashutosh.dixit@xxxxxxxxx>; Andi Shyti
> <andi.shyti@xxxxxxxxxxxxxxx>
> Cc: intel-xe@xxxxxxxxxxxxxxxxxxxxx; linux-hwmon@xxxxxxxxxxxxxxx; Gupta,
> Anshuman <anshuman.gupta@xxxxxxxxx>; linux@xxxxxxxxxxxx; Tauro, Riana
> <riana.tauro@xxxxxxxxx>; Brost, Matthew <matthew.brost@xxxxxxxxx>; Vivi,
> Rodrigo <rodrigo.vivi@xxxxxxxxx>
> Subject: Re: [PATCH v6 5/5] drm/xe/hwmon: Expose power1_max_interval
>
> Hi Ashutosh,
>
> On 27-09-2023 09:02, Dixit, Ashutosh wrote:
> > On Tue, 26 Sep 2023 14:01:06 -0700, Andi Shyti wrote:
> >>
> >
> > Hi Badal/Andi,
> >
> >>
> >>>>>>> + /* val in hw units */
> >>>>>>> + val = DIV_ROUND_CLOSEST_ULL((u64)val << hwmon-
> >scl_shift_time, SF_TIME);
> >>>>>>> + /* Convert to 1.x * power(2,y) */
> >>>>>>> + if (!val) {
> >>>>>>> + /* Avoid ilog2(0) */
> >>>>>>> + y = 0;
> >>>>>>> + x = 0;
> >>>>>>> + } else {
> >>>>>>> + y = ilog2(val);
> >>>>>>> + /* x = (val - (1 << y)) >> (y - 2); */
> >>>>>>
> >>>>>> this is some spurious development comment, can you please remove
> >>>>>> it?
> >>>>>
> >>>>> This is kept intentionally to help to understand the calculations.
> >>>>
> >>>> then this is confusing... Can you please expand the concept?
> >>>> As it is it's not understandable and I would expect someone sending
> >>>> a patch with title:
> >>>>
> >>>> [PATCH] drm/xe/hwmon: Remove spurious comment
> >>>>
> >>>> Because it just looks forgotten from previous development.
> >>> I will add this comment inside the comment at the top of if. So it
> >>> will look like.
> >>> /*
> >>> * Convert to 1.x * power(2,y)
> >>> * y = ilog(val);
> >>> * x = (val - (1 << y)) >> (y-2);
> >>> */
> >>
> >> All right.
> >
> > That comment is explaining that one line of code. I think we should
> > just leave it where it is, it doesn't make sense to move it above the
> > if. How else can we write it so that the comment doesn't look like a leftover?
> >
> > If the code is clear we can remove the comment, but I think the code
> > is hard to understand. So try to understand the code and then you will
> > need the comment.
> Agreed, I will keep this comment as it is.
> >
> >>
> >>>>>>> + x = (val - (1ul << y)) << x_w >> y;
> >>>>>>> + }
> >>>>>>> +
> >>>>>>> + rxy = REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_X, x) |
> >>>>>>> +REG_FIELD_PREP(PKG_PWR_LIM_1_TIME_Y, y);
> >>>>>>> +
> >>>>>>> + xe_device_mem_access_get(gt_to_xe(hwmon->gt));
> >>>>>>> +
> >>>>>>> + mutex_lock(&hwmon->hwmon_lock);
> >>>>>>> +
> >>>>>>> + xe_hwmon_process_reg(hwmon, REG_PKG_RAPL_LIMIT,
> REG_RMW, (u32 *)&r,
> >>>>>>> + PKG_PWR_LIM_1_TIME, rxy);
> >>>>>>> +
> >>>>>>> + mutex_unlock(&hwmon->hwmon_lock);
> >>>>>>
> >>>>>> why are we locking here?
> >>>>>
> >>>>> Since it is rmw operation we are using lock here.
> >>>>
> >>>> OK... so what you are trying to protect here is the
> >>>>
> >>>> read -> update -> write
> >>>>
> >>>> and it makes sense. The problem is that if this is a generic rule,
> >>>> which means that everyone who will do a rmw operation has to take
> >>>> the lock, why not take the lock directly in xe_hwmon_process_reg()?
> >>>>
> >>>> But also this can be a bit confusing, because a function is either
> >>>> locked or unlocked and purists might complain.
> >>>>
> >>>> A suggestion would be to do something like:
> >>>>
> >>>> static int xe_hwmon_process_reg(..., enum xe_hwmon_reg_operation
> operation)
> >>>> {
> >>>> ...
> >>>> }
> >>>>
> >>>> static int xe_hwmon_reg_read(...);
> >>>> {
> >>>> return xe_hwmon_process_reg(..., REG_READ);
> >>>> }
> >>>>
> >>>> static int xe_hwmon_reg_write(...);
> >>>> {
> >>>> return xe_hwmon_process_reg(..., REG_WRITE);
> >>>> }
> >>>>
> >>>> static int xe_hwmon_reg_rmw(...);
> >>>> {
> >>>> int ret;
> >>>>
> >>>> /*
> >>>> * Optional: you can check that the lock is not taken
> >>>> * to shout loud if potential deadlocks arise.
> >>>> */
> >>>>
> >>>> /*
> >>>> * We want to protect the register update with the
> >>>> * lock blah blah blah... explanatory comment.
> >>>> */
> >>>> mutex_lock(&hwmon->hwmon_lock);
> >>>> ret = xe_hwmon_process_reg(..., REG_RMW);
> >>>> mutex_unlock(&hwmon->hwmon_lock);
> >>>>
> >>>> return ret;
> >>>> }
> >>>>
> >>>> What do you think? It looks much clearer to me.
> >>>
> >>> REG_PKG_RAPL_LIMIT register is being written in
> >>> xe_hwmon_power_max_write also, that's why lock is taken. But some
> >>> how while cleaning up I forgot to take it in
> >>> xe_hwmon_power_max_write(), thanks for catching it up. I will update
> xe_hwmon_power_max_write() and resend series.
> >>
> >> yes... OK... then, please add the lock also in the write case.
> >>
> >> But still... thinking of hwmon running in more threads don't you
> >> think we might need a more generic locking? This might mean to lock
> >> all around xe_hwmon_process_reg()... maybe it's an overkill.
> >>
> >> For the time being I'm OK with your current solution.
> >>
> >> If you don't like my suggestion above, feel free to ignore it.
> >
> > Yeah agree, might as well take the lock around the switch statement in
> > xe_hwmon_process_reg().
> Will there be a possibility where two different threads will access power
> attributes power1_max and power1_max_interval simultaneously and
> frequently. I am not able to think such scenario. If not then we can remove
> lock from here.
There are read and write cases, as far as I can see the seq_read_iter always takes seq_file->lock
So read cases like hwm_energy won't need any lock in my opinion, we are protected by above sysfs layer.
https://elixir.bootlin.com/linux/latest/source/fs/seq_file.c#L171
But seq_write on another hand does not use any lock, so I also fees for any ATTR does any read/write operation
on REG_PKG_RAPL_LIMIT register need a lock here.
Thanks,
Anshuman Gupta.
>
> Regards.
> Badal
> >
> >>
> >> Andi
> >
> > Thanks.
> > --
> > Ashutosh
