Re: [PATCH v2] hwmon: add Corsair PSU HID controller driver

[Guenter Roeck]

On Mon, Oct 26, 2020 at 06:35:04PM +0100, Wilken Gottwalt wrote:
> On Mon, 26 Oct 2020 08:25:15 -0700
> Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
> 
> > > Changes in v2:
> > > 	- changed comments to hwmon style comments
> > 
> > This is not "hwmon style". Please read and follow
> > Documentation/process/coding-style.rst.
> >
> > > +	/* at the start of the reply is an echo of the send command/length in the same order
> > > it	*/
> > > +	/* was send, not every command is supported on every device class, if a command is
> > > not	*/
> > > +	/* supported, the length value in the reply is okay, but the command value is set to
> > > 0	*/
> > 
> > Please read and follow Documentation/process/coding-style.rst
> > for multi-line comments.
> 
> Sorry for being a bit slow here. I did read this but I also used other
> drivers as a reference, which I shouldn't have done. The next time it
> will be correct.
> 
> > > +		*val = corsairpsu_linear11_to_int(*val) * 1000;
> > 
> > Something is wrong here. val is a pointer to u32.
> > corsairpsu_linear11_to_int(), however, returns an int.
> > The implementation of corsairpsu_linear11_to_int() suggests
> > that the value can be negative. Either it can be negative
> > and needs to be returned as such, or it can't and
> > corsairpsu_linear11_to_int() should only return unsigned
> > values.
> >
> > Also, this can overflow: corsairpsu_linear11_to_int()
> > can return a value larger that (MAXINT / 1000).
> >
> > > +	case PSU_CMD_TOTAL_WATTS:
> > > +		*val = (data[1] << 8) + data[0];
> > 
> > Same here.
> > 
> > > +		*val = corsairpsu_linear11_to_int(*val) * 1000000;
> > 
> > Same problems as above, and overflow is even more likely.
> 
> You are right, using int here is missleading. The values delivered through
> the LINEAR11 format can be negative, but the micro-controller delivers only
> positive values for the commands used. The biggest LINEAR11 value delivered
> by the micro-controller is about 1600 (1.6 kilo watts for the AX1600i, which
> the driver does not support). The other big value is the fan rpm value, which
> does not exceed 8000. So I'm not sure how to proceed here. Do I make a comment
> explaining this or do I go for the int/long? Using int would limit the uptime
> to, oh, 68 years. I guess going for the int/long will be okay.
> 

Just use long. FWIW, if you only want to handle unsigned values, the return
value (0 or positive) could be combined with the error (negative), and the
pointer would not be needed. I think it better to play safe and return the
value as long *, though.

> > > +			ret = corsairpsu_get_value(priv, PSU_CMD_TOTAL_WATTS, 0, &tmp);
> > > +			break;
> > > +		case 1:
> > > +		case 2:
> > > +		case 3:
> > 
> > What is the problem with the following ?
> > 
> > 		case 1...3:
> >
> 
> Oh, this is a bit embarrassing. I never used (and saw) this switch case
> variant before. Well, you never stop learning...
> 
I have to admit that it is quite new for me as well, but I find it convenient.
And it is kind of odd tht it needs three dots (and it may need spaces before
and after the '...').

> Thank you for your patience, that were some tough lessons.

My pleasure.

Thanks,
Guenter