Patchwork bna: Implement ethtool flash_device entry point.

login
register
mail settings
Submitter krishna gudipati
Date Feb. 1, 2012, 12:06 a.m.
Message ID <1328054801-11322-1-git-send-email-kgudipat@brocade.com>
Download mbox | patch
Permalink /patch/138885/
State Changes Requested
Delegated to: David Miller
Headers show

Comments

krishna gudipati - Feb. 1, 2012, 12:06 a.m.
From: Krishna Gudipati <kgudipat@brocade.com>

Change details:
	- Implement ethtool flash_device() entry point to write the
	  firmware image to the flash firmware partition.

Signed-off-by: Krishna Gudipati <kgudipat@brocade.com>
---
 drivers/net/ethernet/brocade/bna/bfa_defs.h     |    1 +
 drivers/net/ethernet/brocade/bna/bnad_ethtool.c |   36 +++++++++++++++++++++++
 2 files changed, 37 insertions(+), 0 deletions(-)
Ben Hutchings - Feb. 1, 2012, 7:14 p.m.
On Tue, 2012-01-31 at 16:06 -0800, kgudipat@brocade.com wrote:
> From: Krishna Gudipati <kgudipat@brocade.com>
> 
> Change details:
> 	- Implement ethtool flash_device() entry point to write the
> 	  firmware image to the flash firmware partition.
> 
> Signed-off-by: Krishna Gudipati <kgudipat@brocade.com>
> ---
>  drivers/net/ethernet/brocade/bna/bfa_defs.h     |    1 +
>  drivers/net/ethernet/brocade/bna/bnad_ethtool.c |   36 +++++++++++++++++++++++
>  2 files changed, 37 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/ethernet/brocade/bna/bfa_defs.h b/drivers/net/ethernet/brocade/bna/bfa_defs.h
> index 871c630..48f8773 100644
> --- a/drivers/net/ethernet/brocade/bna/bfa_defs.h
> +++ b/drivers/net/ethernet/brocade/bna/bfa_defs.h
> @@ -297,6 +297,7 @@ enum bfa_mode {
>  #define BFA_FLASH_PART_ENTRY_SIZE	32	/* partition entry size */
>  #define BFA_FLASH_PART_MAX		32	/* maximal # of partitions */
>  #define BFA_TOTAL_FLASH_SIZE		0x400000
> +#define BFA_FLASH_PART_FWIMG		2
>  #define BFA_FLASH_PART_MFG		7
>  
>  /*
> diff --git a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
> index 9b44ec8..3e18ee7 100644
> --- a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
> +++ b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
> @@ -1072,6 +1072,41 @@ done:
>  	return ret;
>  }
>  
> +static int
> +bnad_flash_device(struct net_device *netdev, struct ethtool_flash *eflash)
> +{
> +	struct bnad *bnad = netdev_priv(netdev);
> +	struct bnad_iocmd_comp fcomp;
> +	const struct firmware *fw;
> +	unsigned long flags = 0;
> +	int ret = 0;
> +
> +	if (request_firmware(&fw, eflash->data, &bnad->pcidev->dev)) {
> +		pr_err("Can't locate firmware %s\n", eflash->data);
> +		goto out;

This results in returning 0.

> +	}
> +
> +	fcomp.bnad = bnad;
> +	fcomp.comp_status = 0;
> +
> +	init_completion(&fcomp.comp);
> +	spin_lock_irqsave(&bnad->bna_lock, flags);

No need to save flags, this is not called in IRQ context!

> +	ret = bfa_nw_flash_update_part(&bnad->bna.flash, BFA_FLASH_PART_FWIMG,
> +				bnad->id, (u8 *)fw->data, fw->size, 0,
> +				bnad_cb_completion, &fcomp);
> +	if (ret != BFA_STATUS_OK) {
> +		spin_unlock_irqrestore(&bnad->bna_lock, flags);
> +		goto out;
> +	}
> +
> +	spin_unlock_irqrestore(&bnad->bna_lock, flags);
> +	wait_for_completion(&fcomp.comp);
> +	ret = fcomp.comp_status;
> +out:
> +	release_firmware(fw);
> +	return ret;

It looks like the value of ret will be an enumerator from enum
bfa_status and not a Linux error code.  In that case you need to convert
it before returning.

Ben.

> +}
> +
>  static const struct ethtool_ops bnad_ethtool_ops = {
>  	.get_settings = bnad_get_settings,
>  	.set_settings = bnad_set_settings,
> @@ -1090,6 +1125,7 @@ static const struct ethtool_ops bnad_ethtool_ops = {
>  	.get_eeprom_len = bnad_get_eeprom_len,
>  	.get_eeprom = bnad_get_eeprom,
>  	.set_eeprom = bnad_set_eeprom,
> +	.flash_device = bnad_flash_device,
>  };
>  
>  void
krishna gudipati - Feb. 1, 2012, 10:10 p.m.
Thanks Ben for the review.

I am making the changes as per the review comments to return proper error code.

> +	init_completion(&fcomp.comp);

> +	spin_lock_irqsave(&bnad->bna_lock, flags);


No need to save flags, this is not called in IRQ context!

[Krishna]: bna_lock is used in Interrupt context as well, by using spin_lock_irq instead of
	     spin_lock_irqsave() - we will lose the state of the interrupts & will
	     unconditionally enable interrupts. We plan to stay with using spin_lock_irqsave()
	     as it's safe.

Thanks,
Krishna.
Ben Hutchings - Feb. 1, 2012, 10:17 p.m.
On Wed, 2012-02-01 at 14:10 -0800, Krishna Gudipati wrote:
> Thanks Ben for the review.
> 
> I am making the changes as per the review comments to return proper error code.
> 
> > +	init_completion(&fcomp.comp);
> > +	spin_lock_irqsave(&bnad->bna_lock, flags);
> 
> No need to save flags, this is not called in IRQ context!
> 
> [Krishna]: bna_lock is used in Interrupt context as well, by using spin_lock_irq instead of
> 	     spin_lock_irqsave() - we will lose the state of the interrupts & will
> 	     unconditionally enable interrupts. We plan to stay with using spin_lock_irqsave()
> 	     as it's safe.

bnad_flash_device() will be called in process context, therefore it is
correct to unconditionally enable interrupts.  It is safe but odd to use
spin_lock_irqsave()/spin_lock_irqrestore() in such a function.

Ben.

Patch

diff --git a/drivers/net/ethernet/brocade/bna/bfa_defs.h b/drivers/net/ethernet/brocade/bna/bfa_defs.h
index 871c630..48f8773 100644
--- a/drivers/net/ethernet/brocade/bna/bfa_defs.h
+++ b/drivers/net/ethernet/brocade/bna/bfa_defs.h
@@ -297,6 +297,7 @@  enum bfa_mode {
 #define BFA_FLASH_PART_ENTRY_SIZE	32	/* partition entry size */
 #define BFA_FLASH_PART_MAX		32	/* maximal # of partitions */
 #define BFA_TOTAL_FLASH_SIZE		0x400000
+#define BFA_FLASH_PART_FWIMG		2
 #define BFA_FLASH_PART_MFG		7
 
 /*
diff --git a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
index 9b44ec8..3e18ee7 100644
--- a/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
+++ b/drivers/net/ethernet/brocade/bna/bnad_ethtool.c
@@ -1072,6 +1072,41 @@  done:
 	return ret;
 }
 
+static int
+bnad_flash_device(struct net_device *netdev, struct ethtool_flash *eflash)
+{
+	struct bnad *bnad = netdev_priv(netdev);
+	struct bnad_iocmd_comp fcomp;
+	const struct firmware *fw;
+	unsigned long flags = 0;
+	int ret = 0;
+
+	if (request_firmware(&fw, eflash->data, &bnad->pcidev->dev)) {
+		pr_err("Can't locate firmware %s\n", eflash->data);
+		goto out;
+	}
+
+	fcomp.bnad = bnad;
+	fcomp.comp_status = 0;
+
+	init_completion(&fcomp.comp);
+	spin_lock_irqsave(&bnad->bna_lock, flags);
+	ret = bfa_nw_flash_update_part(&bnad->bna.flash, BFA_FLASH_PART_FWIMG,
+				bnad->id, (u8 *)fw->data, fw->size, 0,
+				bnad_cb_completion, &fcomp);
+	if (ret != BFA_STATUS_OK) {
+		spin_unlock_irqrestore(&bnad->bna_lock, flags);
+		goto out;
+	}
+
+	spin_unlock_irqrestore(&bnad->bna_lock, flags);
+	wait_for_completion(&fcomp.comp);
+	ret = fcomp.comp_status;
+out:
+	release_firmware(fw);
+	return ret;
+}
+
 static const struct ethtool_ops bnad_ethtool_ops = {
 	.get_settings = bnad_get_settings,
 	.set_settings = bnad_set_settings,
@@ -1090,6 +1125,7 @@  static const struct ethtool_ops bnad_ethtool_ops = {
 	.get_eeprom_len = bnad_get_eeprom_len,
 	.get_eeprom = bnad_get_eeprom,
 	.set_eeprom = bnad_set_eeprom,
+	.flash_device = bnad_flash_device,
 };
 
 void