From: Jon Mason <jon.mason@exar.com>
To: Ben Hutchings <bhutchings@solarflare.com>
Cc: David Miller <davem@davemloft.net>,
"netdev@vger.kernel.org" <netdev@vger.kernel.org>,
Sivakumar Subramani <Sivakumar.Subramani@exar.com>,
Sreenivasa Honnur <Sreenivasa.Honnur@exar.com>,
Ramkrishna Vepa <Ramkrishna.Vepa@exar.com>
Subject: Re: [PATCH 05/11] vxge: add support for ethtool firmware flashing
Date: Mon, 1 Nov 2010 23:04:56 -0500 [thread overview]
Message-ID: <20101102040456.GC7938@exar.com> (raw)
In-Reply-To: <1288644687.2231.46.camel@achroite.uk.solarflarecom.com>
On Mon, Nov 01, 2010 at 01:51:27PM -0700, Ben Hutchings wrote:
> On Thu, 2010-10-28 at 21:19 -0500, Jon Mason wrote:
> > Add the ability in the vxge driver to flash firmware via ethtool.
> [...]
> > +enum vxge_hw_status
> > +vxge_update_fw_image(struct __vxge_hw_device *hldev, const u8 *fwdata, int size)
> > +{
> > + u64 data0 = 0, data1 = 0, steer_ctrl = 0;
> > + struct __vxge_hw_virtualpath *vpath;
> > + enum vxge_hw_status status;
> > + int ret_code, sec_code, i;
> > +
> > + vpath = &hldev->virtual_paths[hldev->first_vp_id];
> > +
> > + /* send upgrade start command */
> > + status = vxge_hw_vpath_fw_api(vpath,
> > + VXGE_HW_FW_UPGRADE_ACTION,
> > + VXGE_HW_FW_UPGRADE_MEMO,
> > + VXGE_HW_FW_UPGRADE_OFFSET_START,
> > + &data0, &data1, &steer_ctrl);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR, " %s: Upgrade start cmd failed",
> > + __func__);
> > + return status;
> > + }
> > +
> > + /* Transfer fw image to adapter 16 bytes at a time */
> > + for (; size > 0; size -= VXGE_HW_FW_UPGRADE_BLK_SIZE) {
> > + data0 = data1 = steer_ctrl = 0;
> > +
> > + /* send 16 bytes at a time */
> > + for (i = 0; i < VXGE_HW_BYTES_PER_U64; i++) {
> > + data0 |= (u64)fwdata[i] << (i * VXGE_HW_BYTES_PER_U64);
>
> You apparently mean to multiply i by the number of bits in a byte here.
> This happens to be equal to VXGE_HW_BYTES_PER_U64 but it still doesn't
> make sense to use that name for it.
This is transfering the firmware image 128bits at a time. If the name
is confusing, would a "sizeof(u64)" make more sense?
>
> [...]
> > diff --git a/drivers/net/vxge/vxge-ethtool.c b/drivers/net/vxge/vxge-ethtool.c
> > index 6194fd9..c5ab375 100644
> > --- a/drivers/net/vxge/vxge-ethtool.c
> > +++ b/drivers/net/vxge/vxge-ethtool.c
> > @@ -11,7 +11,7 @@
> > * Virtualized Server Adapter.
> > * Copyright(c) 2002-2010 Exar Corp.
> > ******************************************************************************/
> > -#include<linux/ethtool.h>
> > +#include <linux/ethtool.h>
> > #include <linux/slab.h>
> > #include <linux/pci.h>
> > #include <linux/etherdevice.h>
> > @@ -29,7 +29,6 @@
> > * Return value:
> > * 0 on success.
> > */
> > -
> > static int vxge_ethtool_sset(struct net_device *dev, struct ethtool_cmd *info)
> > {
> > /* We currently only support 10Gb/FULL */
> > @@ -148,8 +147,7 @@ static void vxge_ethtool_gregs(struct net_device *dev,
> > static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_flick_link_led(hldev, VXGE_FLICKER_ON);
> > msleep_interruptible(data ? (data * HZ) : VXGE_MAX_FLICKER_TIME);
> > @@ -168,11 +166,10 @@ static int vxge_ethtool_idnic(struct net_device *dev, u32 data)
> > * void
> > */
> > static void vxge_ethtool_getpause_data(struct net_device *dev,
> > - struct ethtool_pauseparam *ep)
> > + struct ethtool_pauseparam *ep)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_getpause_data(hldev, 0, &ep->tx_pause, &ep->rx_pause);
> > }
> > @@ -188,11 +185,10 @@ static void vxge_ethtool_getpause_data(struct net_device *dev,
> > * int, returns 0 on Success
> > */
> > static int vxge_ethtool_setpause_data(struct net_device *dev,
> > - struct ethtool_pauseparam *ep)
> > + struct ethtool_pauseparam *ep)
> > {
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = (struct __vxge_hw_device *)
> > - pci_get_drvdata(vdev->pdev);
> > + struct __vxge_hw_device *hldev = vdev->devh;
> >
> > vxge_hw_device_setpause_data(hldev, 0, ep->tx_pause, ep->rx_pause);
> >
> > @@ -211,7 +207,7 @@ static void vxge_get_ethtool_stats(struct net_device *dev,
> > struct vxge_vpath *vpath = NULL;
> >
> > struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > - struct __vxge_hw_device *hldev = vdev->devh;
> > + struct __vxge_hw_device *hldev = vdev->devh;
> > struct vxge_hw_xmac_stats *xmac_stats;
> > struct vxge_hw_device_stats_sw_info *sw_stats;
> > struct vxge_hw_device_stats_hw_info *hw_stats;
>
> These changes seem to be entirely unrelated to the patch description.
Yes, they should've been included in the cleanup patch.
>
> > @@ -1153,6 +1149,25 @@ static int vxge_set_flags(struct net_device *dev, u32 data)
> > return 0;
> > }
> >
> > +static int vxge_fw_flash(struct net_device *dev, struct ethtool_flash *parms)
> > +{
> > + struct vxgedev *vdev = (struct vxgedev *)netdev_priv(dev);
> > +
> > + if (vdev->max_vpath_supported != VXGE_HW_MAX_VIRTUAL_PATHS) {
> > + printk(KERN_INFO "Single Function Mode is required to flash the"
> > + " firmware\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (netif_running(dev)) {
> > + printk(KERN_INFO "Interface %s must be down to flash the "
> > + "firmware\n", dev->name);
> > + return -EINVAL;
> > + }
> > +
> > + return vxge_fw_upgrade(vdev, parms->data, 1);
> > +}
>
> I think EBUSY is a more appropriate error code. There is nothing wrong
> with the arguments, only the current state of the device.
Fair enough
>
> [...]
> > diff --git a/drivers/net/vxge/vxge-main.c b/drivers/net/vxge/vxge-main.c
> > index d977a63..52a48e7 100644
> > --- a/drivers/net/vxge/vxge-main.c
> > +++ b/drivers/net/vxge/vxge-main.c
> [...]
> > @@ -3277,30 +3279,23 @@ vxge_device_unregister(struct __vxge_hw_device *hldev)
> > struct vxgedev *vdev;
> > struct net_device *dev;
> > char buf[IFNAMSIZ];
> > -#if ((VXGE_DEBUG_INIT & VXGE_DEBUG_MASK) || \
> > - (VXGE_DEBUG_ENTRYEXIT & VXGE_DEBUG_MASK))
> > - u32 level_trace;
> > -#endif
> >
> > dev = hldev->ndev;
> > vdev = netdev_priv(dev);
> > -#if ((VXGE_DEBUG_INIT & VXGE_DEBUG_MASK) || \
> > - (VXGE_DEBUG_ENTRYEXIT & VXGE_DEBUG_MASK))
> > - level_trace = vdev->level_trace;
> > -#endif
> > - vxge_debug_entryexit(level_trace,
> > - "%s: %s:%d", vdev->ndev->name, __func__, __LINE__);
> > + vxge_debug_entryexit(vdev->level_trace, "%s: %s:%d", dev->name,
> > + __func__, __LINE__);
> >
> > - memcpy(buf, vdev->ndev->name, IFNAMSIZ);
> > + memcpy(buf, dev->name, IFNAMSIZ);
> >
> > /* in 2.6 will call stop() if device is up */
> > unregister_netdev(dev);
> >
> > flush_scheduled_work();
> >
> > - vxge_debug_init(level_trace, "%s: ethernet device unregistered", buf);
> > - vxge_debug_entryexit(level_trace,
> > - "%s: %s:%d Exiting...", buf, __func__, __LINE__);
> > + vxge_debug_init(vdev->level_trace, "%s: ethernet device unregistered",
> > + buf);
> > + vxge_debug_entryexit(vdev->level_trace, "%s: %s:%d Exiting...", buf,
> > + __func__, __LINE__);
> > }
> >
> > /*
>
> More apparently unrelated changes.
>
> > @@ -3924,6 +3919,142 @@ static inline u32 vxge_get_num_vfs(u64 function_mode)
> > return num_functions;
> > }
> >
> > +int vxge_fw_upgrade(struct vxgedev *vdev, char *fw_name, int override)
> > +{
> > + struct __vxge_hw_device *hldev = vdev->devh;
> > + u32 maj, min, bld, cmaj, cmin, cbld;
> > + enum vxge_hw_status status;
> > + const struct firmware *fw;
> > + int ret;
> > +
> > + ret = request_firmware(&fw, fw_name, &vdev->pdev->dev);
> > + if (ret) {
> > + vxge_debug_init(VXGE_ERR, "%s: Firmware file '%s' not found",
> > + VXGE_DRIVER_NAME, fw_name);
> > + goto out;
> > + }
> > +
> > + /* Load the new firmware onto the adapter */
> > + status = vxge_update_fw_image(hldev, fw->data, fw->size);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR,
> > + "%s: FW image download to adapter failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
>
> Surely EIO or EINVAL.
EIO would make more sense in the case, yes.
>
> > + goto out;
> > + }
> > +
> > + /* Read the version of the new firmware */
> > + status = vxge_hw_upgrade_read_version(hldev, &maj, &min, &bld);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR,
> > + "%s: Upgrade read version failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
>
> EIO.
>
> > + goto out;
> > + }
> > +
> > + cmaj = vdev->config.device_hw_info.fw_version.major;
> > + cmin = vdev->config.device_hw_info.fw_version.minor;
> > + cbld = vdev->config.device_hw_info.fw_version.build;
> > + /* It's possible the version in /lib/firmware is not the latest version.
> > + * If so, we could get into a loop of trying to upgrade to the latest
> > + * and flashing the older version.
> > + */
> > + if (VXGE_FW_VER(maj, min, bld) == VXGE_FW_VER(cmaj, cmin, cbld) &&
> > + !override) {
> > + ret = -EINVAL;
> > + goto out;
> > + }
> > +
> > + printk(KERN_NOTICE "Upgrade to firmware version %d.%d.%d commencing\n",
> > + maj, min, bld);
> > +
> > + /* Flash the adapter with the new firmware */
> > + status = vxge_hw_flash_fw(hldev);
> > + if (status != VXGE_HW_OK) {
> > + vxge_debug_init(VXGE_ERR, "%s: Upgrade commit failed '%s'.",
> > + VXGE_DRIVER_NAME, fw_name);
> > + ret = -EFAULT;
> > + goto out;
> > + }
> > +
> > + printk(KERN_NOTICE "Upgrade of firmware successful! Adapter must be "
> > + "hard reset before using, thus requiring a system reboot or a "
> > + "hotplug event.\n");
>
> Then what is the point of having the driver manage this? And how can
> you be sure the user will even see this message?
The device is unusable at this point. Open will fail and rmmod/insmod
will fail with hardware errors. On a PCI-hotplug system, the driver
could most likely call those functions directly, but this is not
something that should be done and will not work on non-hotplug
systems. I am open to suggestions for alternatives.
>
> [...]
> > +static int vxge_probe_fw_update(struct vxgedev *vdev)
> > +{
> [...]
> > + ret = vxge_fw_upgrade(vdev, fw_name, 0);
> > + /* -EINVAL and -ENOENT are not fatal errors for flashing firmware on
> > + * probe, so ignore them
> > + */
> > + if (ret != -EINVAL && ret != -ENOENT)
> > + return -EFAULT;
> [..]
>
> EIO.
Thanks for the eyes :)
Jon
>
> Ben.
>
> --
> Ben Hutchings, Senior Software Engineer, Solarflare Communications
> Not speaking for my employer; that's the marketing department's job.
> They asked us to note that Solarflare product names are trademarked.
>
next prev parent reply other threads:[~2010-11-02 4:05 UTC|newest]
Thread overview: 14+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-10-29 2:19 [PATCH 01/11] vxge: enable rxhash Jon Mason
2010-10-29 2:19 ` [PATCH 02/11] vxge: Wait for Rx to become idle before reseting or closing Jon Mason
2010-10-29 2:19 ` [PATCH 03/11] vxge: cleanup debug printing and asserts Jon Mason
2010-10-29 2:19 ` [PATCH 04/11] vxge: serialize access to steering control register Jon Mason
2010-10-29 2:19 ` [PATCH 05/11] vxge: add support for ethtool firmware flashing Jon Mason
2010-11-01 20:51 ` Ben Hutchings
2010-11-02 4:04 ` Jon Mason [this message]
2010-10-29 2:19 ` [PATCH 06/11] vxge: add receive hardware timestamping Jon Mason
2010-10-29 2:19 ` [PATCH 07/11] vxge: Handle errors in vxge_hw_vpath_fw_api Jon Mason
2010-10-29 2:19 ` [PATCH 08/11] vxge: Titan1A detection Jon Mason
2010-10-29 2:19 ` [PATCH 09/11] vxge: correct multi-function detection Jon Mason
2010-10-29 2:19 ` [PATCH 10/11] vxge: update Kconfig Jon Mason
2010-10-29 2:19 ` [PATCH 11/11] vxge: update driver version Jon Mason
-- strict thread matches above, loose matches on Subject: below --
2010-11-04 22:51 [PATCH 01/11] vxge: enable rxhash Jon Mason
2010-11-04 22:51 ` [PATCH 05/11] vxge: add support for ethtool firmware flashing Jon Mason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20101102040456.GC7938@exar.com \
--to=jon.mason@exar.com \
--cc=Ramkrishna.Vepa@exar.com \
--cc=Sivakumar.Subramani@exar.com \
--cc=Sreenivasa.Honnur@exar.com \
--cc=bhutchings@solarflare.com \
--cc=davem@davemloft.net \
--cc=netdev@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).