Hi Badal,
> > > > > + /* 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.
> > > > > + 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.
Andi
