Re: [PATCH 16/42]: PCI: PCI Error reporting callbacks

[Greg KH]

On Mon, Nov 07, 2005 at 11:55:41AM -0600, linas wrote:
> On Mon, Nov 07, 2005 at 10:25:39AM +1100, Paul Mackerras was heard to remark:
> > Greg KH writes:
> > 
> > > > +enum pcierr_result {
> > > > +	PCIERR_RESULT_NONE=0,        /* no result/none/not supported in device driver */
> > > > +	PCIERR_RESULT_CAN_RECOVER=1, /* Device driver can recover without slot reset */
> > > > +	PCIERR_RESULT_NEED_RESET,    /* Device driver wants slot to be reset. */
> > > > +	PCIERR_RESULT_DISCONNECT,    /* Device has completely failed, is unrecoverable */
> > > > +	PCIERR_RESULT_RECOVERED,     /* Device driver is fully recovered and operational */
> > > > +};
> > > 
> > > No, do not create new types of error or return codes.  Use the standard
> > > -EFOO values.  You can document what they should each return, and mean,
> > > but do not create new codes.
> > 
> > Actually, these are not error or return codes, but rather requested
> > actions 
> 
> Yes. 

Ok, then make them be stronger, and not return an int, as everyone will
get that wrong.

> In one incarnation, they were #defines.  The enum was supposed to be 
> the return value of the error notification callbacks.  
> 
> I can prepare a new patch: would you prefer:
> 
> 1) lose typing: #defines and int return value?
> 
> 2) strong typing: enum and enum return value?

3) realy strong typing that sparse can detect.

enums don't really work, as you can get away with using an integer and
the compiler will never complain.  Please use a typedef (yeah, I said
typedef) in the way that sparse will catch any bad users of the code.

> I often prefer strong typing.
> 
> And do you want a patch now, or later?

Depends on when you want to see this make it into mainline :)

thanks,

greg k-h
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/