Re: [Etherboot-developers] [PATCH 6/6] vxge driver submission
Brought to you by:
marty_connor,
stefanhajnoczi
From: Masroor V. <Mas...@ne...> - 2010-02-04 04:22:57
|
Hi Stefan, Do you want me to send a patch for below comments on top of my last patch? Or shall I wait till you complete the review and then send a consolidated patch? I do not see that the last patch got applied. Thanks, Masroor -----Original Message----- From: Stefan Hajnoczi [mailto:ste...@gm...] Sent: Wednesday, February 03, 2010 3:19 PM To: Masroor Vettuparambil Cc: eth...@li...; Marty Connor; Sivakumar Subramani; Ramkrishna Vepa Subject: Re: [Etherboot-developers] [PATCH 6/6] vxge driver submission Feedback after looking through the header files and probe()/remove() code paths: +/** + * vxge_assert + * @test: C-condition to check + * @fmt: printf like format string + * + * This function implements traditional assert. By default assertions + * are enabled. It can be disabled by undefining VXGE_DEBUG_ASSERT macro in + * compilation + * time. + */ +#define vxge_assert(test) assert(test) The comments are out of date. The fmt string argument does not exist and VXGE_DEBUG_ASSERT has no effect (building with DEBUG=vxge_config or any object file name will enable asserts). +#define VXGE_LL_WATCH_DOG_TIMEOUT (15 * HZ) There are many unused #defines but I imagine it is convenient to share code with drivers for other platforms. However, gPXE does not #define HZ. Can you remove this #define? + /* fail the driver loading if firmware is outdated */ + if (fw_version_current != VXGE_CERT_FW_VER) { + printf("%s: Adapter's current firmware version: %d.%d.%d\n", + VXGE_DRIVER_NAME, fw_version->major, + fw_version->minor,fw_version->build); + + printf("%s: Upgrade firmware to version %d.%d.%d\n", + VXGE_DRIVER_NAME, VXGE_CERT_FW_VER_MAJOR, + VXGE_CERT_FW_VER_MINOR, VXGE_CERT_FW_VER_BUILD); + + ret = -EACCES; + goto _exit1; + } This is a very specific test and will fail when you ship a new firmware release. The Linux driver only checks that the major version is 1. Is it safe to relax the gPXE firmware check too? vxge_hw_device_initialize hldev is leaked when __vxge_hw_device_reg_addr_get() fails. The Linux driver appears to have the same bug. vxge_device_register register_netdev() expects hw_addr to be a valid MAC address. register_netdev() initializes ll_addr based on hw_addr. The vxge code does not fill in the MAC address before calling register_netdev(). Instead, hw_addr and ll_addr are filled in manually in vxge_probe() after vxge_device_register has finished. Please fill in hw_addr before calling netdev_register(). There is no need to fill in ll_addr since register_netdev() will do that. vxge_remove + vxge_device_unregister(hldev); + iounmap(vdev->bar0); The vxge_device_unregister() function will free the struct netdev and struct vxgedev, which were allocated together using alloc_etherdev(). Since the struct vxgedev is freed by vxge_device_unregister()/netdev_put(), the vdev->bar0 field should not be accessed afterwards. Stefan |