RE: [PATCH 1/1] drivers: misc: Add support for nCipher HSM devices

[Kim, David]

Hi Greg,

I'm just about to push v2 of our patch but wanted to address your comments in the emails you raised them in.

> > +
> > +/* Device ioctl struct definitions */
> > +
> > +/* Result of the ENQUIRY ioctl. */
> > +struct nfdev_enquiry_str {
> > +	__u32 busno; /**< Which bus is the PCI device on. */
> > +	__u8 slotno; /**< Which slot is the PCI device in. */
> > +	__u8 reserved[3]; /**< for consistent struct alignment */
> 
> What is this crazy /**< text?
> 
> Please use correct kerneldoc formatting to help document things.

Done. I've also double checked all the other files. Checkpatch didn't seem to complain about these so we missed them, sorry.

> > + */
> > +#define NFDEV_CONTROL_HARMLESS(c) ((c) <= 1)
> 
> Why does userspace need this?
> 

It doesn't. We've removed that and a couple other leftovers.

> > +enum {
> > +	/** Enquiry ioctl.
> > +	 *  \return nfdev_enquiry_str describing the attached device.
> > +	 */
> > +	NFDEV_IOCTL_NUM_ENQUIRY = 1,
> > +
> > +	/** Channel Update ioctl.
> > +	 *  \deprecated
> > +	 */
> > +	NFDEV_IOCTL_NUM_CHUPDATE,
> 
> You have to explicitly set your enums if you want them to work properly in an
> ioctl.  As it is, this will fail in nasty ways you can never debug :(
> 

Thanks, we've set them explicitly and also removed the unnecessary ones.

> > +
> > +#define NFDEV_IOCTL_DEBUG                                                      \
> > +	_IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_DEBUG, int)
> 
> Why do you care about debugging to userspace through an ioctl?  Just use
> debugfs and be done with it, that's what it is there for.  Also you can use
> dynamic debugging (hopefully you already are) and use that kernel-wide
> api/interface.
> 
> Individual drivers should NEVER have custom debugging controls and macros.

Yes, this was accidentally left in. It's been removed now.

> 
> > +
> > +#define NFDEV_IOCTL_PCI_IFVERS                                                 \
> > +	_IOW(NFDEV_IOCTL_TYPE, NFDEV_IOCTL_NUM_PCI_IFVERS, int)
> 
> Can't you get this from the pci device information?
> 

Unfortunately not. Each time the device is brought up or has its state changed, the IFVERS is re-negotiated. This call is our means of discovering what IFVER is supported by the currently running firmware.

Thanks,
Dave