[Intel-xe] [PATCH v6 5/5] drm/xe/hwmon: Expose power1_max_interval

[Andi Shyti]

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.

> > > +		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.

Andi