[ofa-general] Re: [PATCH] mlx4: make firmware diagnostic counters available via sysfs

[Roland Dreier]

 > +int mlx4_query_diag_counters(struct mlx4_dev *dev, int array_length,
 > +			     int in_modifier, unsigned int in_offset[],
 > +			     u32 counter_out[])
 > +{
 > +	struct mlx4_cmd_mailbox *mailbox;
 > +	u32 *outbox;
 > +	u32 op_modifer = (u32)in_modifier;

This coding style looks strange to me... you have an int parameter
in_modifier that is not used for anything except to assign it to a u32
op_modifer [sic] variable with a (u32) cast that doesn't do anything.

Why not just have op_modifier be the parameter in the first place?

Also the array_length stuff looks kind of funny since you only ever pass
in a value of 1... why not just pass in int offset and u32 *counter?

 > +	/* clear counters file, can't read it */
 > +	if(offset < 0)
 > +		return sprintf(buf,"This file is write only\n");

Why not just set the permissions on the file so it can't be opened for
reading?  This just looks like a recipe for making userspace code go
crazy on unexpected input.

Also watch out for the space in "if ("

And if I'm understanding correctly, you use a magic offset of -1 for the
clear_diag attribute that makes mlx4_query_diag_counters() read before
the beginning of the output mailbox.

 > +err_diag:
 > +	ib_unregister_device(&ibdev->ib_dev);
 > +
 >  err_reg:
 >  	ib_unregister_device(&ibdev->ib_dev);

This doesn't look like a good idea.

 - R.