| Message ID | 1304989352-24810-4-git-send-email-anirban.chakraborty@qlogic.com |
|---|---|
| State | Superseded, archived |
| Delegated to: | David Miller |
| Headers | show |
On Mon, 2011-05-09 at 18:02 -0700, Anirban Chakraborty wrote: > Driver checks if the previous dump has been cleared before taking the dump. > It doesn't take the dump if it is not cleared. > > Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > --- > drivers/net/qlcnic/qlcnic_ethtool.c | 60 +++++++++++++++++++++++++++++++++++ > 1 files changed, 60 insertions(+), 0 deletions(-) > > diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c > index c541461..1237449 100644 > --- a/drivers/net/qlcnic/qlcnic_ethtool.c > +++ b/drivers/net/qlcnic/qlcnic_ethtool.c > @@ -965,6 +965,64 @@ static void qlcnic_set_msglevel(struct net_device *netdev, u32 msglvl) > adapter->msg_enable = msglvl; > } > > +static int > +qlcnic_get_dump(struct net_device *netdev, struct ethtool_dump *dump, > + void *buffer) > +{ > + int i, copy_sz; > + u32 *hdr_ptr, *data; > + struct qlcnic_adapter *adapter = netdev_priv(netdev); > + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; > + > + if (dump->type == ETHTOOL_DUMP_FLAG) { > + dump->len = fw_dump->tmpl_hdr->size + fw_dump->size; > + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; > + return 0; > + } > + if (!fw_dump->clr) { > + netdev_info(netdev, "Dump not available\n"); > + return -EINVAL; > + } > + copy_sz = fw_dump->tmpl_hdr->size; > + /* Copy template header first */ > + hdr_ptr = (u32 *) fw_dump->tmpl_hdr; > + data = (u32 *) buffer; > + for (i = 0; i < copy_sz/sizeof(u32); i++) > + *data++ = cpu_to_le32(*hdr_ptr++); > + /* Copy captured dump data */ > + memcpy(buffer + copy_sz, fw_dump->data, fw_dump->size); > + dump->len = copy_sz + fw_dump->size; > + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; > + /* free dump area once the whoel dump data has been captured */ > + vfree(fw_dump->data); > + fw_dump->size = 0; > + fw_dump->data = NULL; > + fw_dump->clr = 0; This doesn't seem to be serialised with the code that captures firmware dumps. They need to use the same lock! > + return 0; > +} > + > +static int > +qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val) > +{ > + struct qlcnic_adapter *adapter = netdev_priv(netdev); > + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; > + if (val->flag == QLCNIC_FORCE_FW_DUMP_KEY) { > + netdev_info(netdev, "Forcing a FW dump\n"); > + qlcnic_dev_request_reset(adapter); > + } else { > + if (val->flag > QLCNIC_DUMP_MASK_MAX || > + val->flag < QLCNIC_DUMP_MASK_MIN) { > + netdev_info(netdev, > + "Invalid dump level: 0x%x\n", val->flag); > + return -EINVAL; > + } > + fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff; > + netdev_info(netdev, "Driver mask changed to: 0x%x\n", > + fw_dump->tmpl_hdr->drv_cap_mask); If the flags change, doesn't this invalidate any dump that has been collected by the driver but not saved? Also, same locking problem here. Ben. > + } > + return 0; > +} > + > const struct ethtool_ops qlcnic_ethtool_ops = { > .get_settings = qlcnic_get_settings, > .set_settings = qlcnic_set_settings, > @@ -991,4 +1049,6 @@ const struct ethtool_ops qlcnic_ethtool_ops = { > .set_phys_id = qlcnic_set_led, > .set_msglevel = qlcnic_set_msglevel, > .get_msglevel = qlcnic_get_msglevel, > + .get_dump = qlcnic_get_dump, > + .set_dump = qlcnic_set_dump, > };
On May 10, 2011, at 11:40 AM, Ben Hutchings wrote: > On Mon, 2011-05-09 at 18:02 -0700, Anirban Chakraborty wrote: >> Driver checks if the previous dump has been cleared before taking the dump. >> It doesn't take the dump if it is not cleared. >> >> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> >> --- >> drivers/net/qlcnic/qlcnic_ethtool.c | 60 +++++++++++++++++++++++++++++++++++ >> 1 files changed, 60 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c >> index c541461..1237449 100644 >> --- a/drivers/net/qlcnic/qlcnic_ethtool.c >> +++ b/drivers/net/qlcnic/qlcnic_ethtool.c >> @@ -965,6 +965,64 @@ static void qlcnic_set_msglevel(struct net_device *netdev, u32 msglvl) >> adapter->msg_enable = msglvl; >> } >> >> +static int >> +qlcnic_get_dump(struct net_device *netdev, struct ethtool_dump *dump, >> + void *buffer) >> +{ >> + int i, copy_sz; >> + u32 *hdr_ptr, *data; >> + struct qlcnic_adapter *adapter = netdev_priv(netdev); >> + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; >> + >> + if (dump->type == ETHTOOL_DUMP_FLAG) { >> + dump->len = fw_dump->tmpl_hdr->size + fw_dump->size; >> + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; >> + return 0; >> + } >> + if (!fw_dump->clr) { >> + netdev_info(netdev, "Dump not available\n"); >> + return -EINVAL; >> + } >> + copy_sz = fw_dump->tmpl_hdr->size; >> + /* Copy template header first */ >> + hdr_ptr = (u32 *) fw_dump->tmpl_hdr; >> + data = (u32 *) buffer; >> + for (i = 0; i < copy_sz/sizeof(u32); i++) >> + *data++ = cpu_to_le32(*hdr_ptr++); >> + /* Copy captured dump data */ >> + memcpy(buffer + copy_sz, fw_dump->data, fw_dump->size); >> + dump->len = copy_sz + fw_dump->size; >> + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; >> + /* free dump area once the whoel dump data has been captured */ >> + vfree(fw_dump->data); >> + fw_dump->size = 0; >> + fw_dump->data = NULL; >> + fw_dump->clr = 0; > > This doesn't seem to be serialised with the code that captures firmware > dumps. They need to use the same lock! When we take the dump, we bring down the interface. So, ethtool will not get a chance to fetch the dump data while the dump operation is in progress. > >> + return 0; >> +} >> + >> +static int >> +qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val) >> +{ >> + struct qlcnic_adapter *adapter = netdev_priv(netdev); >> + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; >> + if (val->flag == QLCNIC_FORCE_FW_DUMP_KEY) { >> + netdev_info(netdev, "Forcing a FW dump\n"); >> + qlcnic_dev_request_reset(adapter); >> + } else { >> + if (val->flag > QLCNIC_DUMP_MASK_MAX || >> + val->flag < QLCNIC_DUMP_MASK_MIN) { >> + netdev_info(netdev, >> + "Invalid dump level: 0x%x\n", val->flag); >> + return -EINVAL; >> + } >> + fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff; >> + netdev_info(netdev, "Driver mask changed to: 0x%x\n", >> + fw_dump->tmpl_hdr->drv_cap_mask); > > If the flags change, doesn't this invalidate any dump that has been > collected by the driver but not saved? If the flags change, its effect would be relevant from the next dump capture. The flag indicates to the driver to gather FW dump for a specific level of detail. > > Also, same locking problem here. Addressed above. Thanks for reviewing. -Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 2011-05-10 at 14:00 -0700, Anirban Chakraborty wrote: > On May 10, 2011, at 11:40 AM, Ben Hutchings wrote: > > > On Mon, 2011-05-09 at 18:02 -0700, Anirban Chakraborty wrote: > >> Driver checks if the previous dump has been cleared before taking the dump. > >> It doesn't take the dump if it is not cleared. > >> > >> Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> > >> --- > >> drivers/net/qlcnic/qlcnic_ethtool.c | 60 +++++++++++++++++++++++++++++++++++ > >> 1 files changed, 60 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c > >> index c541461..1237449 100644 > >> --- a/drivers/net/qlcnic/qlcnic_ethtool.c > >> +++ b/drivers/net/qlcnic/qlcnic_ethtool.c > >> @@ -965,6 +965,64 @@ static void qlcnic_set_msglevel(struct net_device *netdev, u32 msglvl) > >> adapter->msg_enable = msglvl; > >> } > >> > >> +static int > >> +qlcnic_get_dump(struct net_device *netdev, struct ethtool_dump *dump, > >> + void *buffer) > >> +{ > >> + int i, copy_sz; > >> + u32 *hdr_ptr, *data; > >> + struct qlcnic_adapter *adapter = netdev_priv(netdev); > >> + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; > >> + > >> + if (dump->type == ETHTOOL_DUMP_FLAG) { > >> + dump->len = fw_dump->tmpl_hdr->size + fw_dump->size; > >> + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; > >> + return 0; > >> + } > >> + if (!fw_dump->clr) { > >> + netdev_info(netdev, "Dump not available\n"); > >> + return -EINVAL; > >> + } > >> + copy_sz = fw_dump->tmpl_hdr->size; > >> + /* Copy template header first */ > >> + hdr_ptr = (u32 *) fw_dump->tmpl_hdr; > >> + data = (u32 *) buffer; > >> + for (i = 0; i < copy_sz/sizeof(u32); i++) > >> + *data++ = cpu_to_le32(*hdr_ptr++); > >> + /* Copy captured dump data */ > >> + memcpy(buffer + copy_sz, fw_dump->data, fw_dump->size); > >> + dump->len = copy_sz + fw_dump->size; > >> + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; > >> + /* free dump area once the whoel dump data has been captured */ > >> + vfree(fw_dump->data); > >> + fw_dump->size = 0; > >> + fw_dump->data = NULL; > >> + fw_dump->clr = 0; > > > > This doesn't seem to be serialised with the code that captures firmware > > dumps. They need to use the same lock! > When we take the dump, we bring down the interface. So, ethtool will not get a chance to > fetch the dump data while the dump operation is in progress. [...] The ethtool core generally doesn't care whether the interface is running. Its operations are serialised only by the RTNL lock. The work item you added to extract a dump from the NIC does not take that lock. Ben.
On May 10, 2011, at 2:58 PM, Ben Hutchings wrote: >>>> <snip> >>> >>> This doesn't seem to be serialised with the code that captures firmware >>> dumps. They need to use the same lock! >> When we take the dump, we bring down the interface. So, ethtool will not get a chance to >> fetch the dump data while the dump operation is in progress. > [...] > > The ethtool core generally doesn't care whether the interface is > running. Its operations are serialised only by the RTNL lock. The work > item you added to extract a dump from the NIC does not take that lock. Oh, ok. Thanks for pointing that out. -Anirban -- To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/net/qlcnic/qlcnic_ethtool.c b/drivers/net/qlcnic/qlcnic_ethtool.c index c541461..1237449 100644 --- a/drivers/net/qlcnic/qlcnic_ethtool.c +++ b/drivers/net/qlcnic/qlcnic_ethtool.c @@ -965,6 +965,64 @@ static void qlcnic_set_msglevel(struct net_device *netdev, u32 msglvl) adapter->msg_enable = msglvl; } +static int +qlcnic_get_dump(struct net_device *netdev, struct ethtool_dump *dump, + void *buffer) +{ + int i, copy_sz; + u32 *hdr_ptr, *data; + struct qlcnic_adapter *adapter = netdev_priv(netdev); + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; + + if (dump->type == ETHTOOL_DUMP_FLAG) { + dump->len = fw_dump->tmpl_hdr->size + fw_dump->size; + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; + return 0; + } + if (!fw_dump->clr) { + netdev_info(netdev, "Dump not available\n"); + return -EINVAL; + } + copy_sz = fw_dump->tmpl_hdr->size; + /* Copy template header first */ + hdr_ptr = (u32 *) fw_dump->tmpl_hdr; + data = (u32 *) buffer; + for (i = 0; i < copy_sz/sizeof(u32); i++) + *data++ = cpu_to_le32(*hdr_ptr++); + /* Copy captured dump data */ + memcpy(buffer + copy_sz, fw_dump->data, fw_dump->size); + dump->len = copy_sz + fw_dump->size; + dump->flag = fw_dump->tmpl_hdr->drv_cap_mask; + /* free dump area once the whoel dump data has been captured */ + vfree(fw_dump->data); + fw_dump->size = 0; + fw_dump->data = NULL; + fw_dump->clr = 0; + return 0; +} + +static int +qlcnic_set_dump(struct net_device *netdev, struct ethtool_dump *val) +{ + struct qlcnic_adapter *adapter = netdev_priv(netdev); + struct qlcnic_fw_dump *fw_dump = &adapter->ahw->fw_dump; + if (val->flag == QLCNIC_FORCE_FW_DUMP_KEY) { + netdev_info(netdev, "Forcing a FW dump\n"); + qlcnic_dev_request_reset(adapter); + } else { + if (val->flag > QLCNIC_DUMP_MASK_MAX || + val->flag < QLCNIC_DUMP_MASK_MIN) { + netdev_info(netdev, + "Invalid dump level: 0x%x\n", val->flag); + return -EINVAL; + } + fw_dump->tmpl_hdr->drv_cap_mask = val->flag & 0xff; + netdev_info(netdev, "Driver mask changed to: 0x%x\n", + fw_dump->tmpl_hdr->drv_cap_mask); + } + return 0; +} + const struct ethtool_ops qlcnic_ethtool_ops = { .get_settings = qlcnic_get_settings, .set_settings = qlcnic_set_settings, @@ -991,4 +1049,6 @@ const struct ethtool_ops qlcnic_ethtool_ops = { .set_phys_id = qlcnic_set_led, .set_msglevel = qlcnic_set_msglevel, .get_msglevel = qlcnic_get_msglevel, + .get_dump = qlcnic_get_dump, + .set_dump = qlcnic_set_dump, };
Driver checks if the previous dump has been cleared before taking the dump. It doesn't take the dump if it is not cleared. Signed-off-by: Anirban Chakraborty <anirban.chakraborty@qlogic.com> --- drivers/net/qlcnic/qlcnic_ethtool.c | 60 +++++++++++++++++++++++++++++++++++ 1 files changed, 60 insertions(+), 0 deletions(-)