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.
>
> > > > > > + 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().
>
> Andi
Thanks.
--
Ashutosh
