Re: [PATCH]sk98lin ethtool support

Re: [PATCH]sk98lin ethtool support

[Feldman, Scott]

On Tue, 30 Dec 2003, Mirko Lindner wrote:

> > Make sure you don't duplicate any ethtool functions.  We don't need a
> > NIC-specific diag tool either ;-)  ethtool is the preferred method
> > moving forward, as it's already shipping in most Linux distros.
> 
> Yes, we need it ;) No kidding! This is not a tool for SW checks like
> media, link or driver version checks, but a tool for HW checks like
> register, PROM, MAC, PHY and some other chip and card checks. The
> ethtool is a great tool, but the intention of this tool is not the same.

If the tool reports the results of running the h/w checks, then you can 
use ETHTOOL_TEST.  The summary results of all the tests is reported as 
PASS/FAIL.  Not sure if your tool needs to do more...

-scott

Re: [PATCH]sk98lin ethtool support

[Mirko Lindner]

Feldman, Scott wrote:
> On Tue, 30 Dec 2003, Mirko Lindner wrote:
> 
> 
>>>Make sure you don't duplicate any ethtool functions.  We don't need a
>>>NIC-specific diag tool either ;-)  ethtool is the preferred method
>>>moving forward, as it's already shipping in most Linux distros.
>>
>>Yes, we need it ;) No kidding! This is not a tool for SW checks like
>>media, link or driver version checks, but a tool for HW checks like
>>register, PROM, MAC, PHY and some other chip and card checks. The
>>ethtool is a great tool, but the intention of this tool is not the same.
> 
> 
> If the tool reports the results of running the h/w checks, then you can 
> use ETHTOOL_TEST.  The summary results of all the tests is reported as 
> PASS/FAIL.  Not sure if your tool needs to do more...
> 
> -scott
> 
> 
> 
> 
> <!DSPAM:3ff51cdf53311090219406>
> 

Scott,

thanks for this info, but the tool reports not only the status, but also 
the results of a test (Example: "Register 0xxxx=xxx", PROM info...). 
"Problem" 2: All tests are included in the DIAG tool and not in the 
driver. We have approx. 100 separate tests (over 1000 individual tests) 
and the driver is huge enough (Support for Genesis, Yukon, Yukon-Lite, 
Yukon-Plus and Yukon2 chipsets).

Mirko

[PATCH]sk98lin ethtool support

[Krishnakumar. R]

Hi,

The following patch introduces the 
ethtool support for the sk98lin
driver. Only 4 operations are supported
for now.

The patch is against vanilla 2.6.0.

As I dont have the hardware with me,
I could do only compilation test.
It compiles fine as inbuilt into 
the kernel.

If you find this okay,
please consider it for the 
inclusion in the mainline driver source. 
(Enquiries were there for this feature,
 Felix's mail to netdev.)

Regards,
KK.

Diffstat output
---------------
h/skdrv1st.h |    1 +
skge.c       |   57
++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
2 files changed, 57 insertions(+), 1 deletion(-)

The patch
---------

--- linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.orig.h	2003-12-30
12:07:26.000000000 +0900
+++ linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.h	2003-12-30
12:08:23.000000000 +0900
@@ -143,6 +143,7 @@
 #include <linux/slab.h>
 #include <linux/interrupt.h>
 #include <linux/pci.h>
+#include <linux/ethtool.h>
 #include <asm/byteorder.h>
 #include <asm/bitops.h>
 #include <asm/io.h>
--- linux-2.6.0/drivers/net/sk98lin/skge.orig.c	2003-12-30
12:04:25.000000000 +0900
+++ linux-2.6.0/drivers/net/sk98lin/skge.c	2003-12-30 13:11:17.000000000
+0900
@@ -415,6 +415,7 @@
  *		<linux/slab.h>
  *		<linux/interrupt.h>
  *		<linux/pci.h>
+ *		<linux/ethtool.h>
  *		<asm/byteorder.h>
  *		<asm/bitops.h>
  *		<asm/io.h>
@@ -567,6 +568,8 @@
 static void	StartDrvCleanupTimer(SK_AC *pAC);
 static void	StopDrvCleanupTimer(SK_AC *pAC);
 static int	XmitFrameSG(SK_AC*, TX_PORT*, struct sk_buff*);
+static void	SkGetDrvInfo (struct SK_NET_DEVICE *dev, struct
ethtool_drvinfo *info);
+static SK_U32	SkGetRxCsum(struct SK_NET_DEVICE *dev);
 

/*******************************************************************************
  *
@@ -599,7 +602,12 @@
 /* local variables
**********************************************************/
 static uintptr_t TxQueueAddr[SK_MAX_MACS][2] = {{0x680, 0x600},{0x780,
0x700}};
 static uintptr_t RxQueueAddr[SK_MAX_MACS] = {0x400, 0x480};
-
+static struct ethtool_ops SKEthtoolOps = {
+	.get_drvinfo		= SkGetDrvInfo,	
+	.get_rx_csum		= SkGetRxCsum,
+	.get_tx_csum		= ethtool_op_get_tx_csum,
+	.get_sg	 		= ethtool_op_get_sg,
+};
 

/*****************************************************************************
  *
@@ -704,6 +712,7 @@
 		dev->set_mac_address =	&SkGeSetMacAddr;
 		dev->do_ioctl =		&SkGeIoctl;
 		dev->change_mtu =	&SkGeChangeMtu;
+		dev->ethtool_ops = 	&SKEthtoolOps;
 		dev->flags &= 		~IFF_RUNNING;
 
 #ifdef SK_ZEROCOPY
@@ -948,6 +957,52 @@
 	
 } /* FreeResources */
 
+
+/*****************************************************************************
+ *
+ * 	SkGetDrvInfo - Get the information about the driver (ethtool).
+ *
+ * Description:
+ *	This function would give the driver name, version, bus info and
register
+ *	length to the ethtool query.
+ *
+ * Returns: N/A
+ *	
+ */
+static void SkGetDrvInfo(struct SK_NET_DEVICE *dev, struct
ethtool_drvinfo *info)
+{
+DEV_NET *pNet;
+SK_AC *pAC;
+
+	pNet = (DEV_NET *) dev->priv;
+	pAC = pNet->pAC;
+	strcpy (info->driver, pAC->Name);
+	strcpy (info->version, pAC->Pnmi.pDriverVersion);
+	strcpy (info->bus_info, pci_name(pAC->PciDev));
+} /* SkGetDrvInfo */
+
+
+
+/*****************************************************************************
+ *
+ * 	SkGetRxCsum - Get whether RX Check sum support is there or not.
+ *
+ * Description:
+ *	This function would give whether the driver has rx check sum feature
+ *	supported or not.
+ *
+ * Returns: 1 if Rx Check sum is supported.
+ *	    0 if Rx Check sum is not supported.		
+ */	
+static SK_U32 	SkGetRxCsum(struct SK_NET_DEVICE *dev)
+{
+#ifdef USE_SK_RX_CHECK	
+	return 1;
+#else 
+	return 0;
+#endif
+} /* SkGetRxCsum */
+
 MODULE_AUTHOR("Mirko Lindner <mlindner@syskonnect.de>");
 MODULE_DESCRIPTION("SysKonnect SK-NET Gigabit Ethernet SK-98xx
driver");
 MODULE_LICENSE("GPL");


-- 
Home Page: http://puggy.symonds.net/~krishnakumar/

Re: [PATCH]sk98lin ethtool support

[Mirko Lindner]

Thanks. I'll complete the support as soon I'm back in the office. All 
driver statistics are also available in the the proc system under 
/proc/net/sk98lin/

Mirko


Krishnakumar. R wrote:
> Hi,
> 
> The following patch introduces the 
> ethtool support for the sk98lin
> driver. Only 4 operations are supported
> for now.
> 
> The patch is against vanilla 2.6.0.
> 
> As I dont have the hardware with me,
> I could do only compilation test.
> It compiles fine as inbuilt into 
> the kernel.
> 
> If you find this okay,
> please consider it for the 
> inclusion in the mainline driver source. 
> (Enquiries were there for this feature,
>  Felix's mail to netdev.)
> 
> Regards,
> KK.
> 
> Diffstat output
> ---------------
> h/skdrv1st.h |    1 +
> skge.c       |   57
> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
> 2 files changed, 57 insertions(+), 1 deletion(-)
> 
> The patch
> ---------
> 
> --- linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.orig.h	2003-12-30
> 12:07:26.000000000 +0900
> +++ linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.h	2003-12-30
> 12:08:23.000000000 +0900
> @@ -143,6 +143,7 @@
>  #include <linux/slab.h>
>  #include <linux/interrupt.h>
>  #include <linux/pci.h>
> +#include <linux/ethtool.h>
>  #include <asm/byteorder.h>
>  #include <asm/bitops.h>
>  #include <asm/io.h>
> --- linux-2.6.0/drivers/net/sk98lin/skge.orig.c	2003-12-30
> 12:04:25.000000000 +0900
> +++ linux-2.6.0/drivers/net/sk98lin/skge.c	2003-12-30 13:11:17.000000000
> +0900
> @@ -415,6 +415,7 @@
>   *		<linux/slab.h>
>   *		<linux/interrupt.h>
>   *		<linux/pci.h>
> + *		<linux/ethtool.h>
>   *		<asm/byteorder.h>
>   *		<asm/bitops.h>
>   *		<asm/io.h>
> @@ -567,6 +568,8 @@
>  static void	StartDrvCleanupTimer(SK_AC *pAC);
>  static void	StopDrvCleanupTimer(SK_AC *pAC);
>  static int	XmitFrameSG(SK_AC*, TX_PORT*, struct sk_buff*);
> +static void	SkGetDrvInfo (struct SK_NET_DEVICE *dev, struct
> ethtool_drvinfo *info);
> +static SK_U32	SkGetRxCsum(struct SK_NET_DEVICE *dev);
>  
> 
> /*******************************************************************************
>   *
> @@ -599,7 +602,12 @@
>  /* local variables
> **********************************************************/
>  static uintptr_t TxQueueAddr[SK_MAX_MACS][2] = {{0x680, 0x600},{0x780,
> 0x700}};
>  static uintptr_t RxQueueAddr[SK_MAX_MACS] = {0x400, 0x480};
> -
> +static struct ethtool_ops SKEthtoolOps = {
> +	.get_drvinfo		= SkGetDrvInfo,	
> +	.get_rx_csum		= SkGetRxCsum,
> +	.get_tx_csum		= ethtool_op_get_tx_csum,
> +	.get_sg	 		= ethtool_op_get_sg,
> +};
>  
> 
> /*****************************************************************************
>   *
> @@ -704,6 +712,7 @@
>  		dev->set_mac_address =	&SkGeSetMacAddr;
>  		dev->do_ioctl =		&SkGeIoctl;
>  		dev->change_mtu =	&SkGeChangeMtu;
> +		dev->ethtool_ops = 	&SKEthtoolOps;
>  		dev->flags &= 		~IFF_RUNNING;
>  
>  #ifdef SK_ZEROCOPY
> @@ -948,6 +957,52 @@
>  	
>  } /* FreeResources */
>  
> +
> +/*****************************************************************************
> + *
> + * 	SkGetDrvInfo - Get the information about the driver (ethtool).
> + *
> + * Description:
> + *	This function would give the driver name, version, bus info and
> register
> + *	length to the ethtool query.
> + *
> + * Returns: N/A
> + *	
> + */
> +static void SkGetDrvInfo(struct SK_NET_DEVICE *dev, struct
> ethtool_drvinfo *info)
> +{
> +DEV_NET *pNet;
> +SK_AC *pAC;
> +
> +	pNet = (DEV_NET *) dev->priv;
> +	pAC = pNet->pAC;
> +	strcpy (info->driver, pAC->Name);
> +	strcpy (info->version, pAC->Pnmi.pDriverVersion);
> +	strcpy (info->bus_info, pci_name(pAC->PciDev));
> +} /* SkGetDrvInfo */
> +
> +
> +
> +/*****************************************************************************
> + *
> + * 	SkGetRxCsum - Get whether RX Check sum support is there or not.
> + *
> + * Description:
> + *	This function would give whether the driver has rx check sum feature
> + *	supported or not.
> + *
> + * Returns: 1 if Rx Check sum is supported.
> + *	    0 if Rx Check sum is not supported.		
> + */	
> +static SK_U32 	SkGetRxCsum(struct SK_NET_DEVICE *dev)
> +{
> +#ifdef USE_SK_RX_CHECK	
> +	return 1;
> +#else 
> +	return 0;
> +#endif
> +} /* SkGetRxCsum */
> +
>  MODULE_AUTHOR("Mirko Lindner <mlindner@syskonnect.de>");
>  MODULE_DESCRIPTION("SysKonnect SK-NET Gigabit Ethernet SK-98xx
> driver");
>  MODULE_LICENSE("GPL");
> 
> 

Re: [PATCH]sk98lin ethtool support

[Felix Radensky]

Thanks, Krishna and Mirco. That would be a great addition to the driver.

I think having most driver info available via ethtool is a very important,
at least for systems with multiple different NICs, as this allows 
information
gathering via single, consistent interface.

Felix.

Mirko Lindner wrote:

> Thanks. I'll complete the support as soon I'm back in the office. All 
> driver statistics are also available in the the proc system under 
> /proc/net/sk98lin/
>
> Mirko
>
>
> Krishnakumar. R wrote:
>
>> Hi,
>>
>> The following patch introduces the ethtool support for the sk98lin
>> driver. Only 4 operations are supported
>> for now.
>>
>> The patch is against vanilla 2.6.0.
>>
>> As I dont have the hardware with me,
>> I could do only compilation test.
>> It compiles fine as inbuilt into the kernel.
>>
>> If you find this okay,
>> please consider it for the inclusion in the mainline driver source. 
>> (Enquiries were there for this feature,
>>  Felix's mail to netdev.)
>>
>> Regards,
>> KK.
>>
>> Diffstat output
>> ---------------
>> h/skdrv1st.h |    1 +
>> skge.c       |   57
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>> 2 files changed, 57 insertions(+), 1 deletion(-)
>>
>> The patch
>> ---------
>>
>> --- linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.orig.h    2003-12-30
>> 12:07:26.000000000 +0900
>> +++ linux-2.6.0/drivers/net/sk98lin/h/skdrv1st.h    2003-12-30
>> 12:08:23.000000000 +0900
>> @@ -143,6 +143,7 @@
>>  #include <linux/slab.h>
>>  #include <linux/interrupt.h>
>>  #include <linux/pci.h>
>> +#include <linux/ethtool.h>
>>  #include <asm/byteorder.h>
>>  #include <asm/bitops.h>
>>  #include <asm/io.h>
>> --- linux-2.6.0/drivers/net/sk98lin/skge.orig.c    2003-12-30
>> 12:04:25.000000000 +0900
>> +++ linux-2.6.0/drivers/net/sk98lin/skge.c    2003-12-30 
>> 13:11:17.000000000
>> +0900
>> @@ -415,6 +415,7 @@
>>   *        <linux/slab.h>
>>   *        <linux/interrupt.h>
>>   *        <linux/pci.h>
>> + *        <linux/ethtool.h>
>>   *        <asm/byteorder.h>
>>   *        <asm/bitops.h>
>>   *        <asm/io.h>
>> @@ -567,6 +568,8 @@
>>  static void    StartDrvCleanupTimer(SK_AC *pAC);
>>  static void    StopDrvCleanupTimer(SK_AC *pAC);
>>  static int    XmitFrameSG(SK_AC*, TX_PORT*, struct sk_buff*);
>> +static void    SkGetDrvInfo (struct SK_NET_DEVICE *dev, struct
>> ethtool_drvinfo *info);
>> +static SK_U32    SkGetRxCsum(struct SK_NET_DEVICE *dev);
>>  
>>
>> /******************************************************************************* 
>>
>>   *
>> @@ -599,7 +602,12 @@
>>  /* local variables
>> **********************************************************/
>>  static uintptr_t TxQueueAddr[SK_MAX_MACS][2] = {{0x680, 0x600},{0x780,
>> 0x700}};
>>  static uintptr_t RxQueueAddr[SK_MAX_MACS] = {0x400, 0x480};
>> -
>> +static struct ethtool_ops SKEthtoolOps = {
>> +    .get_drvinfo        = SkGetDrvInfo,   
>> +    .get_rx_csum        = SkGetRxCsum,
>> +    .get_tx_csum        = ethtool_op_get_tx_csum,
>> +    .get_sg             = ethtool_op_get_sg,
>> +};
>>  
>>
>> /***************************************************************************** 
>>
>>   *
>> @@ -704,6 +712,7 @@
>>          dev->set_mac_address =    &SkGeSetMacAddr;
>>          dev->do_ioctl =        &SkGeIoctl;
>>          dev->change_mtu =    &SkGeChangeMtu;
>> +        dev->ethtool_ops =     &SKEthtoolOps;
>>          dev->flags &=         ~IFF_RUNNING;
>>  
>>  #ifdef SK_ZEROCOPY
>> @@ -948,6 +957,52 @@
>>     
>>  } /* FreeResources */
>>  
>> +
>> +/***************************************************************************** 
>>
>> + *
>> + *     SkGetDrvInfo - Get the information about the driver (ethtool).
>> + *
>> + * Description:
>> + *    This function would give the driver name, version, bus info and
>> register
>> + *    length to the ethtool query.
>> + *
>> + * Returns: N/A
>> + *   
>> + */
>> +static void SkGetDrvInfo(struct SK_NET_DEVICE *dev, struct
>> ethtool_drvinfo *info)
>> +{
>> +DEV_NET *pNet;
>> +SK_AC *pAC;
>> +
>> +    pNet = (DEV_NET *) dev->priv;
>> +    pAC = pNet->pAC;
>> +    strcpy (info->driver, pAC->Name);
>> +    strcpy (info->version, pAC->Pnmi.pDriverVersion);
>> +    strcpy (info->bus_info, pci_name(pAC->PciDev));
>> +} /* SkGetDrvInfo */
>> +
>> +
>> +
>> +/***************************************************************************** 
>>
>> + *
>> + *     SkGetRxCsum - Get whether RX Check sum support is there or not.
>> + *
>> + * Description:
>> + *    This function would give whether the driver has rx check sum 
>> feature
>> + *    supported or not.
>> + *
>> + * Returns: 1 if Rx Check sum is supported.
>> + *        0 if Rx Check sum is not supported.       
>> + */   
>> +static SK_U32     SkGetRxCsum(struct SK_NET_DEVICE *dev)
>> +{
>> +#ifdef USE_SK_RX_CHECK   
>> +    return 1;
>> +#else +    return 0;
>> +#endif
>> +} /* SkGetRxCsum */
>> +
>>  MODULE_AUTHOR("Mirko Lindner <mlindner@syskonnect.de>");
>>  MODULE_DESCRIPTION("SysKonnect SK-NET Gigabit Ethernet SK-98xx
>> driver");
>>  MODULE_LICENSE("GPL");
>>
>>
>

Re: [PATCH]sk98lin ethtool support

[Jeff Garzik]

Mirko Lindner wrote:
> Thanks. I'll complete the support as soon I'm back in the office. All 
> driver statistics are also available in the the proc system under 
> /proc/net/sk98lin/


procfs statistics are deprecated...

	Jeff

Re: [PATCH]sk98lin ethtool support

[Mirko Lindner]

Jeff Garzik wrote:
> Mirko Lindner wrote:
> 
>> Thanks. I'll complete the support as soon I'm back in the office. All 
>> driver statistics are also available in the the proc system under 
>> /proc/net/sk98lin/
> 
> 
> 
> procfs statistics are deprecated...
> 
>     Jeff

Jeff,

the proc statistics are integrated in the driver since kernel 2.2. I'll 
include full support for ethtool as soon I've time to do this. At the 
moment I'm still working on a support for the upcoming Marvell chipset 
(it would be great to support Linux as soon the chip is availiable) and 
a Diag-Tool for all Genesis and Yukon based cards (GUI and console tool).


Mirko

Re: [PATCH]sk98lin ethtool support

[Jeff Garzik]

Mirko Lindner wrote:
> Jeff Garzik wrote:
> 
>> Mirko Lindner wrote:
>>
>>> Thanks. I'll complete the support as soon I'm back in the office. All 
>>> driver statistics are also available in the the proc system under 
>>> /proc/net/sk98lin/
>>
>>
>>
>>
>> procfs statistics are deprecated...
>>
>>     Jeff
> 
> 
> Jeff,
> 
> the proc statistics are integrated in the driver since kernel 2.2. I'll 

Well, understood, but we don't need vendor-specific, non-standard 
statistics when there is a standard method to export these statistics 
(ETHTOOl_GSTATS).


> include full support for ethtool as soon I've time to do this. At the 
> moment I'm still working on a support for the upcoming Marvell chipset 
> (it would be great to support Linux as soon the chip is availiable) and 
> a Diag-Tool for all Genesis and Yukon based cards (GUI and console tool).

Make sure you don't duplicate any ethtool functions.  We don't need a 
NIC-specific diag tool either ;-)  ethtool is the preferred method 
moving forward, as it's already shipping in most Linux distros.

	Jeff

Re: [PATCH]sk98lin ethtool support

[Mirko Lindner]

Jeff,

> Well, understood, but we don't need vendor-specific, non-standard 
> statistics when there is a standard method to export these statistics 
> (ETHTOOl_GSTATS).

I agree with you.


> Make sure you don't duplicate any ethtool functions.  We don't need a 
> NIC-specific diag tool either ;-)  ethtool is the preferred method 
> moving forward, as it's already shipping in most Linux distros.

Yes, we need it ;) No kidding! This is not a tool for SW checks like 
media, link or driver version checks, but a tool for HW checks like 
register, PROM, MAC, PHY and some other chip and card checks. The 
ethtool is a great tool, but the intention of this tool is not the same.

Mirko