Re: [PATCH 6/10] VIOC: New Network Device Driver

[Misha Tomushev <misha@fabric7.com>]

On Sunday 08 October 2006 12:27 am, Pavel Machek wrote:
> Hi!
>
> > +	ecmd->phy_address = 0;	/* !!! Stole from e1000 */
> > +	ecmd->speed = 3;	/* !!! Stole from e1000 */
>
> Eh?
You are right. Will fix.
>
> > +static void vnic_get_regs(struct net_device *netdev,
> > +			  struct ethtool_regs *regs, void *p)
> > +{
> > +	struct vnic_device *vnicdev = netdev->priv;
> > +	struct vioc_device *viocdev = vnicdev->viocdev;
> > +	char *regs_buff = p;
> > +
> > +	memset(regs_buff, 0, VNIC_REGS_CNT * VNIC_REGS_LINE_LEN);
> > +
> > +	regs->version = 1;
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_GLOBAL, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_GLOBAL, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_DEBUG, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_DEBUG, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_DEBUGPRIV, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_DEBUGPRIV, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_FABRIC, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_FABRIC, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_VNIC_EN, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_VNIC_EN, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_PORT_EN, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_PORT_EN, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_BMC_VNIC_CFG, VIOC_BMC, 0),
> > +		VIOC_READ_REG(VREG_BMC_VNIC_CFG, VIOC_BMC, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_IHCU_RXDQEN, VIOC_IHCU, 0),
> > +		VIOC_READ_REG(VREG_IHCU_RXDQEN, VIOC_IHCU, 0, viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_VENG_VLANTAG, VIOC_VENG, vnicdev->vnic_id),
> > +		VIOC_READ_REG(VREG_VENG_VLANTAG, VIOC_VENG, vnicdev->vnic_id,
> > +			      viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +	sprintf(regs_buff, "%08Lx = %08x\n",
> > +		GETRELADDR(VREG_VENG_TXD_CTL, VIOC_VENG, vnicdev->vnic_id),
> > +		VIOC_READ_REG(VREG_VENG_TXD_CTL, VIOC_VENG, vnicdev->vnic_id,
> > +			      viocdev));
> > +	regs_buff += strlen(regs_buff);
> > +
> > +}
>
> This looks ugly. What interface is that?
This is the interface between  the driver and ethtool.
Using the text buffer is one way to keep changed limited to one side (driver). Ultimately, I think that this ethtool function (dumping hw registers) should become more generic,
as opposed to what it is now - unique for every individual driver.
> 							Pavel

-- 
Misha Tomushev
misha@fabric7.com