Re: [PATCH v3 4/4] can: kvaser_usb: Add support for the Usbcan-II family

On 01/12/2015 01:26 PM, Ahmed S. Darwish wrote:

On Mon, Jan 12, 2015 at 12:51:49PM +0100, Marc Kleine-Budde wrote:

quoted

On 01/08/2015 04:19 PM, Ahmed S. Darwish wrote:

[...]

quoted

quoted

quoted

 MODULE_DEVICE_TABLE(usb, kvaser_usb_table);

@@ -463,7 +631,18 @@ static int kvaser_usb_get_software_info(struct kvaser_usb *dev)
 	if (err)
 		return err;
 
-	dev->fw_version = le32_to_cpu(msg.u.softinfo.fw_version);
+	switch (dev->family) {
+	case KVASER_LEAF:
+		dev->fw_version = le32_to_cpu(msg.u.leaf.softinfo.fw_version);
+		break;
+	case KVASER_USBCAN:
+		dev->fw_version = le32_to_cpu(msg.u.usbcan.softinfo.fw_version);
+		break;
+	default:
+		dev_err(dev->udev->dev.parent,
+			"Invalid device family (%d)\n", dev->family);
+		return -EINVAL;

The default case should not happen. I think you can remove it.

quoted

It's true, it _should_ never happen. But I only add such checks if
the follow-up code critically depends on a certain `dev->family`
behavior. So it's kind of a defensive check against any possible
bug in driver or memory.

What do you think?

The kernel is full of callback functions, if you have a bit flip there
you're in trouble anyways. A bug in the driver (or other parts of the
kernel) might overwrite the memory of dev->family, but if this happens,
more things will break.

I see. Thanks for the explanation.

Most of them are now removed in latest submission, except two to
avoid GCC warnings of variables that "may be used uninitialized".

Thanks, I'll look at the code later and try to figure out what gcc's
problem might be.

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |