[Open-FCoE] [PATCH] libhbalinux: use ethtool to get driver name and version

Joe Eykholt jeykholt at cisco.com
Tue Jan 6 20:07:32 UTC 2009


Hi Yi and all.

I'm taking this opportunity to re-raise some higher-level questions about the
role of this library, as well as to pick some nits about the patch.  Feel free
to ignore the nits.  The code looks fine as far as it goes.

Yi Zou wrote:
> I came across this issue after having my nic driver as built-in, so fix this
> by instead of using sys/class/net/%ifname/device/driver/module, changing
> to query by ethtool, o.w., if the driver is built-in, the above path is invalid.

So what good is that path if it can't be used in all kernel configurations?
Don't device modules get a link in the built-in cases?

> also, since this info. is not critical, do not fail the query even if the
> driver and version info. is not available, just return w/ dummy info.

I agree.

> Signed-off-by: Yi Zou <yi.zou at intel.com>
> ---
> 
>  src/adapt_impl.h |    2 -
>  src/lport.c      |   84 ++++++++++++++++++++++++++++++++++++++----------------
>  2 files changed, 59 insertions(+), 27 deletions(-)
> 
> diff --git a/src/adapt_impl.h b/src/adapt_impl.h
> index 17875e5..dce518b 100644
> --- a/src/adapt_impl.h
> +++ b/src/adapt_impl.h
> @@ -22,8 +22,6 @@
>  #define SYSFS_HOST_DIR     "/sys/class/fc_host"
>  #define SYSFS_HBA_DIR      "/sys/class/net"
>  #define SYSFS_LUN_DIR      "/sys/class/scsi_device"
> -#define SYSFS_MODULE       "/driver/module"
> -#define SYSFS_MODULE_VER   "driver/module/version"
>  #define SYSFS_RPORT_ROOT       "/sys/class/fc_remote_ports"
>  #define SYSFS_RPORT_DIR        "rport-%u:%u-%u" /* host, chan, rport */
>  
> diff --git a/src/lport.c b/src/lport.c
> index 9c47887..ec855cf 100644
> --- a/src/lport.c
> +++ b/src/lport.c
> @@ -19,6 +19,11 @@
>  #include "utils.h"
>  #include "api_lib.h"
>  #include "adapt_impl.h"
> +#include <net/if.h>
> +#include <sys/socket.h>
> +#include <sys/ioctl.h>
> +#include <linux/ethtool.h>
> +#include <linux/sockios.h>
>  
>  #ifndef HBA_STATUS_ERROR_ILLEGAL_FCID
>  #define HBA_STATUS_ERROR_ILLEGAL_FCID 33	/* defined after HBA-API 2.2 */
> @@ -98,6 +103,57 @@ counting_rports(struct dirent *dp, void *arg)
>  	return HBA_STATUS_OK;
>  }
>  
> +#define DUMMY_DRVNAME "name: n/a"
> +#define DUMMY_DRVVER "version: n/a"
> +/*
> + * get_drvinfo - use ethtool get if driver name and version
> + * use this so we can get driver name and version for both module driver or
> + * built-in driver. When this fails, fills the info. w/ dummy string.
> + *
> + * Returns: none
> + */
> +static int get_drvinfo(const char *ifname, HBA_ADAPTERATTRIBUTES *atp)
> +{
> +	int s;
> +	struct ifreq rq;
> +	struct ethtool_drvinfo di;
> +
> +	/* maybe a built-in w/o module info in sysfs, try ethtool */
> +	s = socket(AF_INET, SOCK_DGRAM, 0);
> +	if (s == -1) {
> +		fprintf(stderr, "%s: ioctl(SIOCETHTOOL) failed, errno=0x%x\n",
> +				__func__, errno);

Above: line wrap style 1.   __func__ should be indented by tabs or under "stderr".

I don't think we want to print anything here.  Returning the "n/a" is enough info.

> +		goto use_dummy;
> +	}
> +	memset(&di, 0, sizeof(di));
> +	di.cmd = ETHTOOL_GDRVINFO;
> +
> +	memset(&rq, 0, sizeof(rq));
> +	strncpy(rq.ifr_name, ifname, sizeof(rq.ifr_name)-1);

Put a space around the '-'.  Didn't checkpatch catch this?

> +	rq.ifr_data = (char *) &di;
> +
> +	if (ioctl(s, SIOCETHTOOL, &rq) == -1) {
> +		fprintf(stderr, "%s: socket failed, errno=0x%x\n",
> +			__func__, errno);

Above: line wrap style 2.   OK, "__func__" is under stderr.

> +		close(s);
> +		goto use_dummy;
> +	}
> +	sa_strncpy_safe(atp->DriverVersion, sizeof(atp->DriverVersion),
> +		di.version, sizeof(atp->DriverVersion));
> +	sa_strncpy_safe(atp->DriverName, sizeof(atp->DriverName),
> +		di.driver, sizeof(atp->DriverName));

Above: line wrap style 3.  di.driver indented by tabs?

> +	close(s);
> +	return 0;
> +
> +use_dummy:
> +	/* fill dummy info to avoid failing */
> +	sa_strncpy_safe(atp->DriverVersion, sizeof(atp->DriverVersion),
> +			DUMMY_DRVVER, sizeof(atp->DriverVersion));
> +	sa_strncpy_safe(atp->DriverName, sizeof(atp->DriverName),
> +			DUMMY_DRVNAME, sizeof(atp->DriverName));

Line wrap style 2.

> +	return -1;
> +}

The above routine to return void, since that's how it's
used and how it's documented.

Also, this won't work for devices that aren't Ethernet, like fnic.
It seems you're designing this library to be just for fcoe.ko.
Which would require other libfc LLDs to have their own, practically
identical vendor-specific library, so I'd like this library to work
for all libfc LLDs.

Perhaps the driver name/version should reflect that of the LLD,
not the underlying network device.  Since fcoe.ko is more like a protocol,
not a device.

The driver name can be extracted from /sys/class/fc_host/host*/symbolic_name.
We could add the version name there, too.

Please see the patches I submitted on December 4.  Do those look OK?
If so, when will they get committed?

> +
>  static int
>  sysfs_scan(struct dirent *dp, void *arg)
>  {
> @@ -108,9 +164,8 @@ sysfs_scan(struct dirent *dp, void *arg)
>  	struct hba_info hba_info;
>  	struct adapter_info *ap;
>  	struct port_info *pp;
> -	char host_dir[80], hba_dir[80], drv_dir[80];
> +	char host_dir[80], hba_dir[80];
>  	char ifname[20], buf[256];
> -	char *driverName;
>  	int data[32], rc, i;
>  
>  	/* Found a local port! */
> @@ -337,10 +392,6 @@ sysfs_scan(struct dirent *dp, void *arg)
>  	/* Get VendorSpecificID (TODO) */
>  	atp->VendorSpecificID = HBA_VENDOR_SPECIFIC_ID;
>  
> -	/* Get DriverVersion */
> -	rc = sa_sys_read_line(hba_dir, SYSFS_MODULE_VER,
> -			atp->DriverVersion, sizeof(atp->DriverVersion));
> -
>  	/* Get NodeSymbolicName */
>  	sa_strncpy_safe(atp->NodeSymbolicName, sizeof(atp->NodeSymbolicName),
>  			ap->ad_name, sizeof(atp->NodeSymbolicName));
> @@ -351,25 +402,8 @@ sysfs_scan(struct dirent *dp, void *arg)
>  	memcpy((char *)&atp->NodeWWN, (char *)&pap->NodeWWN,
>  		sizeof(pap->NodeWWN));
>  
> -	/* Get DriverName */
> -	snprintf(drv_dir, sizeof(drv_dir), "%s" SYSFS_MODULE , hba_dir);
> -	i = readlink(drv_dir, buf, sizeof(buf));
> -	if (i < 0) {
> -		printf("Fatal! readlink %s failed\n", drv_dir);
> -		return HBA_STATUS_ERROR;
> -	}
> -	buf[i] = '\0';
> -	if (!strstr(buf, "module")) {
> -		/*
> -		 * Does not find "module" in the string.
> -		 * This should not happen. In this case, set
> -		 * the driver name to "Unknown".
> -		 */
> -		driverName = "Unknown";
> -	} else
> -		driverName = strstr(buf, "module") + 7;
> -	sa_strncpy_safe(atp->DriverName, sizeof(atp->DriverName),
> -			driverName, sizeof(atp->DriverName));
> +	/* Get DriverName and DriverVersion */
> +	get_drvinfo(ifname, atp);
>  
>  	/*
>  	 * Give HBA to library
> 

	Regards,
	Joe Eykholt



More information about the devel mailing list