* Re: [RFC 1/6] qed: Add support for hardware offloaded iSCSI.
From: Johannes Thumshirn @ 2016-10-20 7:09 UTC (permalink / raw)
To: Arun Easi
Cc: manish.rangankar, lduncan, cleech, martin.petersen, jejb,
linux-scsi, netdev, Yuval.Mintz, QLogic-Storage-Upstream,
Yuval Mintz
In-Reply-To: <alpine.LRH.2.00.1610191538530.25712@mvluser05.qlc.com>
Hi Arun,
On Wed, Oct 19, 2016 at 05:14:59PM -0700, Arun Easi wrote:
> Thanks Johannes for the review, please see my response below.
>
[...]
> >
> > Why not introduce a small helper like:
> > static inline bool qed_is_iscsi_personality()
> > {
> > return IS_ENABLED(CONFIG_QEDI) && p_hwfn->hw_info.personality ==
> > QED_PCI_ISCSI;
> > }
>
> I think I can remove the IS_ENABLED() check in places like this
> and have the check contained in header file. qed_iscsi_free()
> already is taken care, if I do the same fore qed_ooo*, I think
> the check would just be "p_hwfn->hw_info.personality ==
> QED_PCI_ISCSI", which would keep it consistent with the other
> areas where similar check is done for other protocols.
Sounds good.
>
[...]
> >
> > Is there any chance you could factor out above blocks into own functions so
> > you have
> >
> >
> > if (!GET_FIELD(p_ramrod->iscsi.flags,
> > ISCSI_CONN_OFFLOAD_PARAMS_TCP_ON_CHIP_1B)) {
> > qedi_do_stuff_off_chip();
> > else
> > qedi_do_stuff_on_chip();
> >
>
> This function mostly fills data needed for the firmware interface.
> By having all data necessary for the command
> ISCSI_RAMROD_CMD_ID_OFFLOAD_CONN in this function it is easier to
> refer what is being fed to firmware. If you do not have strong
> objections, I would like to keep it this way.
No strong objections, I just don't think these lengthy blocks are readable,
but I don't want to do too much bikeshedding about it.
Thanks,
Johannes
--
Johannes Thumshirn Storage
jthumshirn@suse.de +49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850
^ permalink raw reply
* [PATCH net-next V2 1/9] liquidio CN23XX: HW config for VF support
From: Raghu Vatsavayi @ 2016-10-20 5:40 UTC (permalink / raw)
To: davem
Cc: netdev, Raghu Vatsavayi, Raghu Vatsavayi, Derek Chickles,
Satanand Burla, Felix Manlunas
In-Reply-To: <1476942046-18789-1-git-send-email-rvatsavayi@caviumnetworks.com>
Adds support for configuring HW for creating VFs.
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@caviumnetworks.com>
Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
Signed-off-by: Satanand Burla <satananda.burla@caviumnetworks.com>
Signed-off-by: Felix Manlunas <felix.manlunas@caviumnetworks.com>
---
.../ethernet/cavium/liquidio/cn23xx_pf_device.c | 125 +++++++++++++++++----
drivers/net/ethernet/cavium/liquidio/lio_main.c | 18 +++
.../net/ethernet/cavium/liquidio/octeon_config.h | 5 +
.../net/ethernet/cavium/liquidio/octeon_device.h | 12 +-
4 files changed, 137 insertions(+), 23 deletions(-)
diff --git a/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c b/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
index 380a641..c7257ed 100644
--- a/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
@@ -309,9 +309,10 @@ u32 cn23xx_pf_get_oq_ticks(struct octeon_device *oct, u32 time_intr_in_us)
static void cn23xx_setup_global_mac_regs(struct octeon_device *oct)
{
- u64 reg_val;
u16 mac_no = oct->pcie_port;
u16 pf_num = oct->pf_num;
+ u64 reg_val;
+ u64 temp;
/* programming SRN and TRS for each MAC(0..3) */
@@ -333,6 +334,14 @@ static void cn23xx_setup_global_mac_regs(struct octeon_device *oct)
/* setting TRS <23:16> */
reg_val = reg_val |
(oct->sriov_info.trs << CN23XX_PKT_MAC_CTL_RINFO_TRS_BIT_POS);
+ /* setting RPVF <39:32> */
+ temp = oct->sriov_info.rings_per_vf & 0xff;
+ reg_val |= (temp << CN23XX_PKT_MAC_CTL_RINFO_RPVF_BIT_POS);
+
+ /* setting NVFS <55:48> */
+ temp = oct->sriov_info.max_vfs & 0xff;
+ reg_val |= (temp << CN23XX_PKT_MAC_CTL_RINFO_NVFS_BIT_POS);
+
/* write these settings to MAC register */
octeon_write_csr64(oct, CN23XX_SLI_PKT_MAC_RINFO64(mac_no, pf_num),
reg_val);
@@ -399,11 +408,12 @@ static int cn23xx_reset_io_queues(struct octeon_device *oct)
static int cn23xx_pf_setup_global_input_regs(struct octeon_device *oct)
{
+ struct octeon_cn23xx_pf *cn23xx = (struct octeon_cn23xx_pf *)oct->chip;
+ struct octeon_instr_queue *iq;
+ u64 intr_threshold, reg_val;
u32 q_no, ern, srn;
u64 pf_num;
- u64 intr_threshold, reg_val;
- struct octeon_instr_queue *iq;
- struct octeon_cn23xx_pf *cn23xx = (struct octeon_cn23xx_pf *)oct->chip;
+ u64 vf_num;
pf_num = oct->pf_num;
@@ -420,6 +430,16 @@ static int cn23xx_pf_setup_global_input_regs(struct octeon_device *oct)
*/
for (q_no = 0; q_no < ern; q_no++) {
reg_val = oct->pcie_port << CN23XX_PKT_INPUT_CTL_MAC_NUM_POS;
+
+ /* for VF assigned queues. */
+ if (q_no < oct->sriov_info.pf_srn) {
+ vf_num = q_no / oct->sriov_info.rings_per_vf;
+ vf_num += 1; /* VF1, VF2,........ */
+ } else {
+ vf_num = 0;
+ }
+
+ reg_val |= vf_num << CN23XX_PKT_INPUT_CTL_VF_NUM_POS;
reg_val |= pf_num << CN23XX_PKT_INPUT_CTL_PF_NUM_POS;
octeon_write_csr64(oct, CN23XX_SLI_IQ_PKT_CONTROL64(q_no),
@@ -1048,49 +1068,110 @@ static void cn23xx_setup_reg_address(struct octeon_device *oct)
static int cn23xx_sriov_config(struct octeon_device *oct)
{
- u32 total_rings;
struct octeon_cn23xx_pf *cn23xx = (struct octeon_cn23xx_pf *)oct->chip;
- /* num_vfs is already filled for us */
+ u32 max_vfs = oct->sriov_info.max_vfs;
+ u32 rings_per_vf, max_possible_vfs;
u32 pf_srn, num_pf_rings;
+ u32 total_rings;
cn23xx->conf =
- (struct octeon_config *)oct_get_config_info(oct, LIO_23XX);
+ (struct octeon_config *)oct_get_config_info(oct, LIO_23XX);
switch (oct->rev_id) {
case OCTEON_CN23XX_REV_1_0:
total_rings = CN23XX_MAX_RINGS_PER_PF_PASS_1_0;
+ max_possible_vfs = CN23XX_MAX_VFS_PER_PF_PASS_1_0;
break;
case OCTEON_CN23XX_REV_1_1:
total_rings = CN23XX_MAX_RINGS_PER_PF_PASS_1_1;
+ max_possible_vfs = CN23XX_MAX_VFS_PER_PF_PASS_1_1;
break;
default:
total_rings = CN23XX_MAX_RINGS_PER_PF;
+ max_possible_vfs = CN23XX_MAX_VFS_PER_PF;
break;
}
- if (!oct->sriov_info.num_pf_rings) {
- if (total_rings > num_present_cpus())
- num_pf_rings = num_present_cpus();
- else
- num_pf_rings = total_rings;
- } else {
- num_pf_rings = oct->sriov_info.num_pf_rings;
- if (num_pf_rings > total_rings) {
- dev_warn(&oct->pci_dev->dev,
- "num_queues_per_pf requested %u is more than available rings. Reducing to %u\n",
- num_pf_rings, total_rings);
- num_pf_rings = total_rings;
+ if (max_vfs > min((total_rings - 1), max_possible_vfs)) {
+ dev_warn(&oct->pci_dev->dev, "max_vfs requested %u is more than available rings. Reducing to %u\n",
+ max_vfs, min((total_rings - 1), max_possible_vfs));
+ max_vfs = min((total_rings - 1), max_possible_vfs);
+ }
+
+ if (!max_vfs) {
+ dev_dbg(&oct->pci_dev->dev, "max_vfs is zero, will derive max_vfs based on number of pf rings\n");
+ rings_per_vf = 0;
+
+ if (!oct->sriov_info.num_pf_rings) {
+ if (total_rings > num_present_cpus())
+ num_pf_rings = num_present_cpus();
+ else
+ num_pf_rings = total_rings;
+ } else {
+ num_pf_rings = oct->sriov_info.num_pf_rings;
+
+ if (num_pf_rings > total_rings) {
+ dev_warn(&oct->pci_dev->dev,
+ "num_queues_per_pf requested %u is more than available rings. Reducing to %u\n",
+ num_pf_rings, total_rings);
+ num_pf_rings = total_rings;
+ }
+ }
+
+ max_vfs = total_rings - num_pf_rings;
+
+ if (max_vfs)
+ rings_per_vf = 1;
+ } else {
+ if (oct->sriov_info.num_pf_rings > 0) {
+ num_pf_rings = oct->sriov_info.num_pf_rings;
+ if (num_pf_rings > (total_rings - max_vfs)) {
+ num_pf_rings = total_rings - max_vfs;
+ dev_warn(&oct->pci_dev->dev,
+ "num_queues_per_pf requested %u is more than available rings. Reducing to %u\n",
+ oct->sriov_info.num_pf_rings,
+ num_pf_rings);
+ }
+ rings_per_vf = rounddown_pow_of_two((total_rings -
+ num_pf_rings) / max_vfs);
+ rings_per_vf = min_t(u32, rings_per_vf,
+ CN23XX_MAX_RINGS_PER_VF);
+ } else {
+ if ((oct->rev_id == OCTEON_CN23XX_REV_1_0) &&
+ (max_vfs >= LIOLUT_RING_DISTRIBUTION)) {
+ rings_per_vf = 1;
+ total_rings = max_vfs + 1;
+ } else if (oct->rev_id == OCTEON_CN23XX_REV_1_0) {
+ rings_per_vf = liolut_num_vfs_to_rings_per_vf
+ [max_vfs];
+ } else {
+ rings_per_vf = rounddown_pow_of_two(
+ total_rings / (max_vfs + 1));
+ }
+ rings_per_vf = min_t(u32, rings_per_vf,
+ CN23XX_MAX_RINGS_PER_VF);
+ num_pf_rings = total_rings - (rings_per_vf * max_vfs);
+
+ if (num_pf_rings > num_present_cpus()) {
+ num_pf_rings = num_present_cpus();
+ total_rings = num_pf_rings +
+ rings_per_vf * max_vfs;
+ }
}
}
- total_rings = num_pf_rings;
+ total_rings = num_pf_rings + (max_vfs * rings_per_vf);
+
/* the first ring of the pf */
pf_srn = total_rings - num_pf_rings;
oct->sriov_info.trs = total_rings;
+ oct->sriov_info.max_vfs = max_vfs;
+ oct->sriov_info.rings_per_vf = rings_per_vf;
oct->sriov_info.pf_srn = pf_srn;
oct->sriov_info.num_pf_rings = num_pf_rings;
- dev_dbg(&oct->pci_dev->dev, "trs:%d pf_srn:%d num_pf_rings:%d\n",
- oct->sriov_info.trs, oct->sriov_info.pf_srn,
+ dev_dbg(&oct->pci_dev->dev, "trs:%d max_vfs:%d rings_per_vf:%d pf_srn:%d num_pf_rings:%d\n",
+ oct->sriov_info.trs, oct->sriov_info.max_vfs,
+ oct->sriov_info.rings_per_vf, oct->sriov_info.pf_srn,
oct->sriov_info.num_pf_rings);
return 0;
}
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 71d01a7..016b7aa 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -69,6 +69,19 @@
module_param(conf_type, int, 0);
MODULE_PARM_DESC(conf_type, "select octeon configuration 0 default 1 ovs");
+/* Default behaviour of Liquidio is to provide one queue per VF. But Liquidio
+ * can also provide multiple queues to each VF. If user wants to change the
+ * default behaviour HW should be provided configuration info at init time,
+ * based on which it will create control queues for communicating with FW.
+ */
+static u32 max_vfs[2] = { 0, 0 };
+module_param_array(max_vfs, int, NULL, 0444);
+MODULE_PARM_DESC(max_vfs, "Assign two comma-separated unsigned integers that specify max number of VFs for PF0 (left of the comma) and PF1 (right of the comma); for 23xx only. By default HW will configure as many VFs as queues after allocating PF queues.To increase queues for VF use this parameter. Use sysfs to create these VFs.");
+
+static unsigned int num_queues_per_pf[2] = { 0, 0 };
+module_param_array(num_queues_per_pf, uint, NULL, 0444);
+MODULE_PARM_DESC(num_queues_per_pf, "two comma-separated unsigned integers that specify number of queues per PF0 (left of the comma) and PF1 (right of the comma); for 23xx only");
+
static int ptp_enable = 1;
/* Bit mask values for lio->ifstate */
@@ -1730,6 +1743,11 @@ static int octeon_chip_specific_setup(struct octeon_device *oct)
case OCTEON_CN23XX_PCIID_PF:
oct->chip_id = OCTEON_CN23XX_PF_VID;
+ oct->sriov_info.max_vfs = max_vfs[oct->pci_dev->devfn];
+ if (num_queues_per_pf[oct->pci_dev->devfn] > 0) {
+ oct->sriov_info.num_pf_rings =
+ num_queues_per_pf[oct->pci_dev->devfn];
+ }
ret = setup_cn23xx_octeon_pf_device(oct);
s = "CN23XX";
break;
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_config.h b/drivers/net/ethernet/cavium/liquidio/octeon_config.h
index c765568..0127a0e 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_config.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_config.h
@@ -65,6 +65,11 @@
#define DEFAULT_NUM_NIC_PORTS_68XX_210NV 2
/* CN23xx IQ configuration macros */
+#define CN23XX_MAX_VFS_PER_PF_PASS_1_0 8
+#define CN23XX_MAX_VFS_PER_PF_PASS_1_1 31
+#define CN23XX_MAX_VFS_PER_PF 63
+#define CN23XX_MAX_RINGS_PER_VF 8
+
#define CN23XX_MAX_RINGS_PER_PF_PASS_1_0 12
#define CN23XX_MAX_RINGS_PER_PF_PASS_1_1 32
#define CN23XX_MAX_RINGS_PER_PF 64
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.h b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
index da15c2a..751d3b6 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
@@ -322,11 +322,21 @@ struct octeon_pf_vf_hs_word {
};
struct octeon_sriov_info {
+ /* Number of rings assigned to VF */
+ u32 rings_per_vf;
+
+ /** Max Number of VF devices that can be enabled. This variable can
+ * specified during load time or it will be derived after allocating
+ * PF queues. When max_vfs is derived then each VF will get one queue
+ **/
+ u32 max_vfs;
+
/* Actual rings left for PF device */
u32 num_pf_rings;
- /* SRN of PF usable IO queues */
+ /* SRN of PF usable IO queues */
u32 pf_srn;
+
/* total pf rings */
u32 trs;
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next V2 0/9] liquidio CN23XX VF support
From: Raghu Vatsavayi @ 2016-10-20 5:40 UTC (permalink / raw)
To: davem; +Cc: netdev, Raghu Vatsavayi
Dave,
Following is the V2 patch series for adding VF support on
CN23XX devices. This version addressed your concern for
ordering of local variable declarations from longest to shortest
line. There are some other minor changes for fixing new checkpatch
script related errors on pre-existing driver.
I will post remaining VF patches soon after this patchseries is
applied. Please apply patches in the following order as some of
the patches depend on earlier patches.
Thanks.
Raghu Vatsavayi (9):
liquidio CN23XX: HW config for VF support
liquidio CN23XX: sysfs VF config support
liquidio CN23XX: Mailbox support
liquidio CN23XX: mailbox interrupt processing
liquidio CN23XX: VF related operations
liquidio CN23XX: device states
liquidio CN23XX: code cleanup
liquidio CN23XX: copyrights changes and alignment
liquidio CN23XX: fix for new check patch errors
drivers/net/ethernet/cavium/liquidio/Makefile | 1 +
.../ethernet/cavium/liquidio/cn23xx_pf_device.c | 360 ++++++++++++++---
.../ethernet/cavium/liquidio/cn23xx_pf_device.h | 42 +-
.../net/ethernet/cavium/liquidio/cn23xx_pf_regs.h | 51 ++-
.../net/ethernet/cavium/liquidio/cn66xx_device.c | 49 +--
.../net/ethernet/cavium/liquidio/cn66xx_device.h | 41 +-
drivers/net/ethernet/cavium/liquidio/cn66xx_regs.h | 49 ++-
.../net/ethernet/cavium/liquidio/cn68xx_device.c | 38 +-
.../net/ethernet/cavium/liquidio/cn68xx_device.h | 37 +-
drivers/net/ethernet/cavium/liquidio/cn68xx_regs.h | 37 +-
drivers/net/ethernet/cavium/liquidio/lio_core.c | 68 +++-
drivers/net/ethernet/cavium/liquidio/lio_ethtool.c | 65 ++-
drivers/net/ethernet/cavium/liquidio/lio_main.c | 437 ++++++++++++++++++---
.../net/ethernet/cavium/liquidio/liquidio_common.h | 100 +++--
.../net/ethernet/cavium/liquidio/liquidio_image.h | 36 +-
.../net/ethernet/cavium/liquidio/octeon_config.h | 45 ++-
.../net/ethernet/cavium/liquidio/octeon_console.c | 156 ++++----
.../net/ethernet/cavium/liquidio/octeon_device.c | 74 ++--
.../net/ethernet/cavium/liquidio/octeon_device.h | 133 ++++---
drivers/net/ethernet/cavium/liquidio/octeon_droq.c | 91 +++--
drivers/net/ethernet/cavium/liquidio/octeon_droq.h | 18 +-
drivers/net/ethernet/cavium/liquidio/octeon_iq.h | 25 +-
.../net/ethernet/cavium/liquidio/octeon_mailbox.c | 318 +++++++++++++++
.../net/ethernet/cavium/liquidio/octeon_mailbox.h | 112 ++++++
drivers/net/ethernet/cavium/liquidio/octeon_main.h | 47 +--
.../net/ethernet/cavium/liquidio/octeon_mem_ops.c | 7 +-
.../net/ethernet/cavium/liquidio/octeon_mem_ops.h | 5 +-
.../net/ethernet/cavium/liquidio/octeon_network.h | 11 +-
drivers/net/ethernet/cavium/liquidio/octeon_nic.c | 5 +-
drivers/net/ethernet/cavium/liquidio/octeon_nic.h | 7 +-
.../net/ethernet/cavium/liquidio/request_manager.c | 34 +-
.../ethernet/cavium/liquidio/response_manager.c | 11 +-
.../ethernet/cavium/liquidio/response_manager.h | 6 +-
33 files changed, 1738 insertions(+), 778 deletions(-)
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
--
1.8.3.1
^ permalink raw reply
* [PATCH net-next V2 2/9] liquidio CN23XX: sysfs VF config support
From: Raghu Vatsavayi @ 2016-10-20 5:40 UTC (permalink / raw)
To: davem
Cc: netdev, Raghu Vatsavayi, Raghu Vatsavayi, Derek Chickles,
Satanand Burla, Felix Manlunas
In-Reply-To: <1476942046-18789-1-git-send-email-rvatsavayi@caviumnetworks.com>
Adds sysfs based support for enabling or disabling VFs.
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@caviumnetworks.com>
Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
Signed-off-by: Satanand Burla <satananda.burla@caviumnetworks.com>
Signed-off-by: Felix Manlunas <felix.manlunas@caviumnetworks.com>
---
.../ethernet/cavium/liquidio/cn23xx_pf_device.c | 3 +
drivers/net/ethernet/cavium/liquidio/lio_main.c | 98 ++++++++++++++++++++++
.../net/ethernet/cavium/liquidio/octeon_config.h | 3 +
.../net/ethernet/cavium/liquidio/octeon_device.h | 8 ++
4 files changed, 112 insertions(+)
diff --git a/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c b/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
index c7257ed..b3fa5d9 100644
--- a/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
+++ b/drivers/net/ethernet/cavium/liquidio/cn23xx_pf_device.c
@@ -1173,6 +1173,9 @@ static int cn23xx_sriov_config(struct octeon_device *oct)
oct->sriov_info.trs, oct->sriov_info.max_vfs,
oct->sriov_info.rings_per_vf, oct->sriov_info.pf_srn,
oct->sriov_info.num_pf_rings);
+
+ oct->sriov_info.sriov_enabled = 0;
+
return 0;
}
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_main.c b/drivers/net/ethernet/cavium/liquidio/lio_main.c
index 016b7aa..2825590 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_main.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_main.c
@@ -193,6 +193,8 @@ struct octeon_device_priv {
unsigned long napi_mask;
};
+static int liquidio_enable_sriov(struct pci_dev *dev, int num_vfs);
+
static int octeon_device_init(struct octeon_device *);
static int liquidio_stop(struct net_device *netdev);
static void liquidio_remove(struct pci_dev *pdev);
@@ -531,6 +533,7 @@ static int liquidio_resume(struct pci_dev *pdev __attribute__((unused)))
.suspend = liquidio_suspend,
.resume = liquidio_resume,
#endif
+ .sriov_configure = liquidio_enable_sriov,
};
/**
@@ -1485,6 +1488,8 @@ static void octeon_destroy_resources(struct octeon_device *oct)
continue;
octeon_delete_instr_queue(oct, i);
}
+ if (oct->sriov_info.sriov_enabled)
+ pci_disable_sriov(oct->pci_dev);
/* fallthrough */
case OCT_DEV_SC_BUFF_POOL_INIT_DONE:
octeon_free_sc_buffer_pool(oct);
@@ -4008,6 +4013,99 @@ static int setup_nic_devices(struct octeon_device *octeon_dev)
return -ENODEV;
}
+static int octeon_enable_sriov(struct octeon_device *oct)
+{
+ unsigned int num_vfs_alloced = oct->sriov_info.num_vfs_alloced;
+ struct pci_dev *vfdev;
+ int err;
+ u32 u;
+
+ if (OCTEON_CN23XX_PF(oct) && num_vfs_alloced) {
+ err = pci_enable_sriov(oct->pci_dev,
+ oct->sriov_info.num_vfs_alloced);
+ if (err) {
+ dev_err(&oct->pci_dev->dev,
+ "OCTEON: Failed to enable PCI sriov: %d\n",
+ err);
+ oct->sriov_info.num_vfs_alloced = 0;
+ return err;
+ }
+ oct->sriov_info.sriov_enabled = 1;
+
+ /* init lookup table that maps DPI ring number to VF pci_dev
+ * struct pointer
+ */
+ u = 0;
+ vfdev = pci_get_device(PCI_VENDOR_ID_CAVIUM,
+ OCTEON_CN23XX_VF_VID, NULL);
+ while (vfdev) {
+ if (vfdev->is_virtfn &&
+ (vfdev->physfn == oct->pci_dev)) {
+ oct->sriov_info.dpiring_to_vfpcidev_lut[u] =
+ vfdev;
+ u += oct->sriov_info.rings_per_vf;
+ }
+ vfdev = pci_get_device(PCI_VENDOR_ID_CAVIUM,
+ OCTEON_CN23XX_VF_VID, vfdev);
+ }
+ }
+
+ return num_vfs_alloced;
+}
+
+static int lio_pci_sriov_disable(struct octeon_device *oct)
+{
+ int u;
+
+ if (pci_vfs_assigned(oct->pci_dev)) {
+ dev_err(&oct->pci_dev->dev, "VFs are still assigned to VMs.\n");
+ return -EPERM;
+ }
+
+ pci_disable_sriov(oct->pci_dev);
+
+ u = 0;
+ while (u < MAX_POSSIBLE_VFS) {
+ oct->sriov_info.dpiring_to_vfpcidev_lut[u] = NULL;
+ u += oct->sriov_info.rings_per_vf;
+ }
+
+ oct->sriov_info.num_vfs_alloced = 0;
+ dev_info(&oct->pci_dev->dev, "oct->pf_num:%d disabled VFs\n",
+ oct->pf_num);
+
+ return 0;
+}
+
+static int liquidio_enable_sriov(struct pci_dev *dev, int num_vfs)
+{
+ struct octeon_device *oct = pci_get_drvdata(dev);
+ int ret = 0;
+
+ if ((num_vfs == oct->sriov_info.num_vfs_alloced) &&
+ (oct->sriov_info.sriov_enabled)) {
+ dev_info(&oct->pci_dev->dev, "oct->pf_num:%d already enabled num_vfs:%d\n",
+ oct->pf_num, num_vfs);
+ return 0;
+ }
+
+ if (!num_vfs) {
+ ret = lio_pci_sriov_disable(oct);
+ } else if (num_vfs > oct->sriov_info.max_vfs) {
+ dev_err(&oct->pci_dev->dev,
+ "OCTEON: Max allowed VFs:%d user requested:%d",
+ oct->sriov_info.max_vfs, num_vfs);
+ ret = -EPERM;
+ } else {
+ oct->sriov_info.num_vfs_alloced = num_vfs;
+ ret = octeon_enable_sriov(oct);
+ dev_info(&oct->pci_dev->dev, "oct->pf_num:%d enabled num_vfs:%d\n",
+ oct->pf_num, num_vfs);
+ }
+
+ return ret;
+}
+
/**
* \brief initialize the NIC
* @param oct octeon device
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_config.h b/drivers/net/ethernet/cavium/liquidio/octeon_config.h
index 0127a0e..8871fb5 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_config.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_config.h
@@ -471,4 +471,7 @@ struct octeon_config {
#define MAX_POSSIBLE_OCTEON_INSTR_QUEUES CN23XX_MAX_INPUT_QUEUES
#define MAX_POSSIBLE_OCTEON_OUTPUT_QUEUES CN23XX_MAX_OUTPUT_QUEUES
+
+#define MAX_POSSIBLE_VFS 64
+
#endif /* __OCTEON_CONFIG_H__ */
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.h b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
index 751d3b6..cfd12ec 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
@@ -38,6 +38,7 @@
#define OCTEON_CN68XX 0x0091
#define OCTEON_CN66XX 0x0092
#define OCTEON_CN23XX_PF_VID 0x9702
+#define OCTEON_CN23XX_VF_VID 0x9712
/**RevisionId for the chips */
#define OCTEON_CN23XX_REV_1_0 0x00
@@ -331,6 +332,9 @@ struct octeon_sriov_info {
**/
u32 max_vfs;
+ /** Number of VF devices enabled using sysfs. */
+ u32 num_vfs_alloced;
+
/* Actual rings left for PF device */
u32 num_pf_rings;
@@ -340,6 +344,10 @@ struct octeon_sriov_info {
/* total pf rings */
u32 trs;
+ u32 sriov_enabled;
+
+ /*lookup table that maps DPI ring number to VF pci_dev struct pointer*/
+ struct pci_dev *dpiring_to_vfpcidev_lut[MAX_POSSIBLE_VFS];
};
struct octeon_ioq_vector {
--
1.8.3.1
^ permalink raw reply related
* [PATCH net-next V2 3/9] liquidio CN23XX: Mailbox support
From: Raghu Vatsavayi @ 2016-10-20 5:40 UTC (permalink / raw)
To: davem
Cc: netdev, Raghu Vatsavayi, Raghu Vatsavayi, Derek Chickles,
Satanand Burla, Felix Manlunas
In-Reply-To: <1476942046-18789-1-git-send-email-rvatsavayi@caviumnetworks.com>
Adds support for mailbox communication between PF and VF.
Signed-off-by: Raghu Vatsavayi <raghu.vatsavayi@caviumnetworks.com>
Signed-off-by: Derek Chickles <derek.chickles@caviumnetworks.com>
Signed-off-by: Satanand Burla <satananda.burla@caviumnetworks.com>
Signed-off-by: Felix Manlunas <felix.manlunas@caviumnetworks.com>
---
drivers/net/ethernet/cavium/liquidio/Makefile | 1 +
drivers/net/ethernet/cavium/liquidio/lio_core.c | 32 ++
.../net/ethernet/cavium/liquidio/liquidio_common.h | 6 +-
.../net/ethernet/cavium/liquidio/octeon_device.h | 4 +
.../net/ethernet/cavium/liquidio/octeon_mailbox.c | 321 +++++++++++++++++++++
.../net/ethernet/cavium/liquidio/octeon_mailbox.h | 115 ++++++++
drivers/net/ethernet/cavium/liquidio/octeon_main.h | 2 +-
7 files changed, 478 insertions(+), 3 deletions(-)
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
create mode 100644 drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
diff --git a/drivers/net/ethernet/cavium/liquidio/Makefile b/drivers/net/ethernet/cavium/liquidio/Makefile
index 5a27b2a..14958de 100644
--- a/drivers/net/ethernet/cavium/liquidio/Makefile
+++ b/drivers/net/ethernet/cavium/liquidio/Makefile
@@ -11,6 +11,7 @@ liquidio-$(CONFIG_LIQUIDIO) += lio_ethtool.o \
cn66xx_device.o \
cn68xx_device.o \
cn23xx_pf_device.o \
+ octeon_mailbox.o \
octeon_mem_ops.o \
octeon_droq.o \
octeon_nic.o
diff --git a/drivers/net/ethernet/cavium/liquidio/lio_core.c b/drivers/net/ethernet/cavium/liquidio/lio_core.c
index 201eddb..e6026df 100644
--- a/drivers/net/ethernet/cavium/liquidio/lio_core.c
+++ b/drivers/net/ethernet/cavium/liquidio/lio_core.c
@@ -264,3 +264,35 @@ void liquidio_link_ctrl_cmd_completion(void *nctrl_ptr)
nctrl->ncmd.s.cmd);
}
}
+
+void octeon_pf_changed_vf_macaddr(struct octeon_device *oct, u8 *mac)
+{
+ bool macaddr_changed = false;
+ struct net_device *netdev;
+ struct lio *lio;
+
+ rtnl_lock();
+
+ netdev = oct->props[0].netdev;
+ lio = GET_LIO(netdev);
+
+ lio->linfo.macaddr_is_admin_asgnd = true;
+
+ if (!ether_addr_equal(netdev->dev_addr, mac)) {
+ macaddr_changed = true;
+ ether_addr_copy(netdev->dev_addr, mac);
+ ether_addr_copy(((u8 *)&lio->linfo.hw_addr) + 2, mac);
+ call_netdevice_notifiers(NETDEV_CHANGEADDR, netdev);
+ }
+
+ rtnl_unlock();
+
+ if (macaddr_changed)
+ dev_info(&oct->pci_dev->dev,
+ "PF changed VF's MAC address to %02hhx:%02hhx:%02hhx:%02hhx:%02hhx:%02hhx\n",
+ mac[0], mac[1], mac[2], mac[3], mac[4], mac[5]);
+
+ /* no need to notify the firmware of the macaddr change because
+ * the PF did that already
+ */
+}
diff --git a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
index 0d990ac..caeff9a 100644
--- a/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
+++ b/drivers/net/ethernet/cavium/liquidio/liquidio_common.h
@@ -731,13 +731,15 @@ struct oct_link_info {
#ifdef __BIG_ENDIAN_BITFIELD
u64 gmxport:16;
- u64 rsvd:32;
+ u64 macaddr_is_admin_asgnd:1;
+ u64 rsvd:31;
u64 num_txpciq:8;
u64 num_rxpciq:8;
#else
u64 num_rxpciq:8;
u64 num_txpciq:8;
- u64 rsvd:32;
+ u64 rsvd:31;
+ u64 macaddr_is_admin_asgnd:1;
u64 gmxport:16;
#endif
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_device.h b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
index cfd12ec..77a6eb7 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_device.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_device.h
@@ -492,6 +492,9 @@ struct octeon_device {
int msix_on;
+ /** Mail Box details of each octeon queue. */
+ struct octeon_mbox *mbox[MAX_POSSIBLE_VFS];
+
/** IOq information of it's corresponding MSI-X interrupt. */
struct octeon_ioq_vector *ioq_vector;
@@ -511,6 +514,7 @@ struct octeon_device {
#define OCTEON_CN6XXX(oct) ((oct->chip_id == OCTEON_CN66XX) || \
(oct->chip_id == OCTEON_CN68XX))
#define OCTEON_CN23XX_PF(oct) (oct->chip_id == OCTEON_CN23XX_PF_VID)
+#define OCTEON_CN23XX_VF(oct) ((oct)->chip_id == OCTEON_CN23XX_VF_VID)
#define CHIP_FIELD(oct, TYPE, field) \
(((struct octeon_ ## TYPE *)(oct->chip))->field)
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
new file mode 100644
index 0000000..3a2f6c1
--- /dev/null
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.c
@@ -0,0 +1,321 @@
+/**********************************************************************
+ * Author: Cavium, Inc.
+ *
+ * Contact: support@cavium.com
+ * Please include "LiquidIO" in the subject.
+ *
+ * Copyright (c) 2003-2016 Cavium, Inc.
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful, but
+ * AS-IS and WITHOUT ANY WARRANTY; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, TITLE, or
+ * NONINFRINGEMENT. See the GNU General Public License for more details.
+ *
+ * This file may also be available under a different license from Cavium.
+ * Contact Cavium, Inc. for more information
+ ***********************************************************************/
+#include <linux/pci.h>
+#include <linux/netdevice.h>
+#include "liquidio_common.h"
+#include "octeon_droq.h"
+#include "octeon_iq.h"
+#include "response_manager.h"
+#include "octeon_device.h"
+#include "octeon_main.h"
+#include "octeon_mailbox.h"
+
+/**
+ * octeon_mbox_read:
+ * @oct: Pointer mailbox
+ *
+ * Reads the 8-bytes of data from the mbox register
+ * Writes back the acknowldgement inidcating completion of read
+ */
+int octeon_mbox_read(struct octeon_mbox *mbox)
+{
+ union octeon_mbox_message msg;
+ int ret = 0;
+
+ spin_lock(&mbox->lock);
+
+ msg.u64 = readq(mbox->mbox_read_reg);
+
+ if ((msg.u64 == OCTEON_PFVFACK) || (msg.u64 == OCTEON_PFVFSIG)) {
+ spin_unlock(&mbox->lock);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVING) {
+ mbox->mbox_req.data[mbox->mbox_req.recv_len - 1] = msg.u64;
+ mbox->mbox_req.recv_len++;
+ } else {
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVING) {
+ mbox->mbox_resp.data[mbox->mbox_resp.recv_len - 1] =
+ msg.u64;
+ mbox->mbox_resp.recv_len++;
+ } else {
+ if ((mbox->state & OCTEON_MBOX_STATE_IDLE) &&
+ (msg.s.type == OCTEON_MBOX_REQUEST)) {
+ mbox->state &= ~OCTEON_MBOX_STATE_IDLE;
+ mbox->state |=
+ OCTEON_MBOX_STATE_REQUEST_RECEIVING;
+ mbox->mbox_req.msg.u64 = msg.u64;
+ mbox->mbox_req.q_no = mbox->q_no;
+ mbox->mbox_req.recv_len = 1;
+ } else {
+ if ((mbox->state &
+ OCTEON_MBOX_STATE_RESPONSE_PENDING) &&
+ (msg.s.type == OCTEON_MBOX_RESPONSE)) {
+ mbox->state &=
+ ~OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ mbox->state |=
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING
+ ;
+ mbox->mbox_resp.msg.u64 = msg.u64;
+ mbox->mbox_resp.q_no = mbox->q_no;
+ mbox->mbox_resp.recv_len = 1;
+ } else {
+ writeq(OCTEON_PFVFERR,
+ mbox->mbox_read_reg);
+ mbox->state |= OCTEON_MBOX_STATE_ERROR;
+ spin_unlock(&mbox->lock);
+ return 1;
+ }
+ }
+ }
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVING) {
+ if (mbox->mbox_req.recv_len < msg.s.len) {
+ ret = 0;
+ } else {
+ mbox->state &= ~OCTEON_MBOX_STATE_REQUEST_RECEIVING;
+ mbox->state |= OCTEON_MBOX_STATE_REQUEST_RECEIVED;
+ ret = 1;
+ }
+ } else {
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVING) {
+ if (mbox->mbox_resp.recv_len < msg.s.len) {
+ ret = 0;
+ } else {
+ mbox->state &=
+ ~OCTEON_MBOX_STATE_RESPONSE_RECEIVING;
+ mbox->state |=
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVED;
+ ret = 1;
+ }
+ } else {
+ WARN_ON(1);
+ }
+ }
+
+ writeq(OCTEON_PFVFACK, mbox->mbox_read_reg);
+
+ spin_unlock(&mbox->lock);
+
+ return ret;
+}
+
+/**
+ * octeon_mbox_write:
+ * @oct: Pointer Octeon Device
+ * @mbox_cmd: Cmd to send to mailbox.
+ *
+ * Populates the queue specific mbox structure
+ * with cmd information.
+ * Write the cmd to mbox register
+ */
+int octeon_mbox_write(struct octeon_device *oct,
+ struct octeon_mbox_cmd *mbox_cmd)
+{
+ struct octeon_mbox *mbox = oct->mbox[mbox_cmd->q_no];
+ u32 count, i, ret = OCTEON_MBOX_STATUS_SUCCESS;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mbox->lock, flags);
+
+ if ((mbox_cmd->msg.s.type == OCTEON_MBOX_RESPONSE) &&
+ !(mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVED)) {
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return OCTEON_MBOX_STATUS_FAILED;
+ }
+
+ if ((mbox_cmd->msg.s.type == OCTEON_MBOX_REQUEST) &&
+ !(mbox->state & OCTEON_MBOX_STATE_IDLE)) {
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return OCTEON_MBOX_STATUS_BUSY;
+ }
+
+ if (mbox_cmd->msg.s.type == OCTEON_MBOX_REQUEST) {
+ memcpy(&mbox->mbox_resp, mbox_cmd,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ }
+
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ count = 0;
+
+ while (readq(mbox->mbox_write_reg) != OCTEON_PFVFSIG) {
+ schedule_timeout_uninterruptible(10);
+ if (count++ == 1000) {
+ ret = OCTEON_MBOX_STATUS_FAILED;
+ break;
+ }
+ }
+
+ if (ret == OCTEON_MBOX_STATUS_SUCCESS) {
+ writeq(mbox_cmd->msg.u64, mbox->mbox_write_reg);
+ for (i = 0; i < (u32)(mbox_cmd->msg.s.len - 1); i++) {
+ count = 0;
+ while (readq(mbox->mbox_write_reg) !=
+ OCTEON_PFVFACK) {
+ schedule_timeout_uninterruptible(10);
+ if (count++ == 1000) {
+ ret = OCTEON_MBOX_STATUS_FAILED;
+ break;
+ }
+ }
+ writeq(mbox_cmd->data[i], mbox->mbox_write_reg);
+ }
+ }
+
+ spin_lock_irqsave(&mbox->lock, flags);
+ if (mbox_cmd->msg.s.type == OCTEON_MBOX_RESPONSE) {
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ } else {
+ if ((!mbox_cmd->msg.s.resp_needed) ||
+ (ret == OCTEON_MBOX_STATUS_FAILED)) {
+ mbox->state &= ~OCTEON_MBOX_STATE_RESPONSE_PENDING;
+ if (!(mbox->state &
+ (OCTEON_MBOX_STATE_REQUEST_RECEIVING |
+ OCTEON_MBOX_STATE_REQUEST_RECEIVED)))
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ }
+ }
+ spin_unlock_irqrestore(&mbox->lock, flags);
+
+ return ret;
+}
+
+/**
+ * octeon_mbox_process_cmd:
+ * @mbox: Pointer mailbox
+ * @mbox_cmd: Pointer to command received
+ *
+ * Process the cmd received in mbox
+ */
+static int octeon_mbox_process_cmd(struct octeon_mbox *mbox,
+ struct octeon_mbox_cmd *mbox_cmd)
+{
+ struct octeon_device *oct = mbox->oct_dev;
+
+ switch (mbox_cmd->msg.s.cmd) {
+ case OCTEON_VF_ACTIVE:
+ dev_dbg(&oct->pci_dev->dev, "got vfactive sending data back\n");
+ mbox_cmd->msg.s.type = OCTEON_MBOX_RESPONSE;
+ mbox_cmd->msg.s.resp_needed = 1;
+ mbox_cmd->msg.s.len = 2;
+ mbox_cmd->data[0] = 0; /* VF version is in mbox_cmd->data[0] */
+ ((struct lio_version *)&mbox_cmd->data[0])->major =
+ LIQUIDIO_BASE_MAJOR_VERSION;
+ ((struct lio_version *)&mbox_cmd->data[0])->minor =
+ LIQUIDIO_BASE_MINOR_VERSION;
+ ((struct lio_version *)&mbox_cmd->data[0])->micro =
+ LIQUIDIO_BASE_MICRO_VERSION;
+ memcpy(mbox_cmd->msg.s.params, (uint8_t *)&oct->pfvf_hsword, 6);
+ /* Sending core cofig info to the corresponding active VF.*/
+ octeon_mbox_write(oct, mbox_cmd);
+ break;
+
+ case OCTEON_VF_FLR_REQUEST:
+ dev_info(&oct->pci_dev->dev,
+ "got a request for FLR from VF that owns DPI ring %u\n",
+ mbox->q_no);
+ pcie_capability_set_word(
+ oct->sriov_info.dpiring_to_vfpcidev_lut[mbox->q_no],
+ PCI_EXP_DEVCTL, PCI_EXP_DEVCTL_BCR_FLR);
+ break;
+
+ case OCTEON_PF_CHANGED_VF_MACADDR:
+ if (OCTEON_CN23XX_VF(oct))
+ octeon_pf_changed_vf_macaddr(oct,
+ mbox_cmd->msg.s.params);
+ break;
+
+ default:
+ break;
+ }
+ return 0;
+}
+
+/**
+ *octeon_mbox_process_message:
+ *
+ * Process the received mbox message.
+ */
+int octeon_mbox_process_message(struct octeon_mbox *mbox)
+{
+ struct octeon_mbox_cmd mbox_cmd;
+ unsigned long flags;
+
+ spin_lock_irqsave(&mbox->lock, flags);
+
+ if (mbox->state & OCTEON_MBOX_STATE_ERROR) {
+ if (mbox->state & (OCTEON_MBOX_STATE_RESPONSE_PENDING |
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING)) {
+ memcpy(&mbox_cmd, &mbox->mbox_resp,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ mbox_cmd.recv_status = 1;
+ if (mbox_cmd.fn)
+ mbox_cmd.fn(mbox->oct_dev, &mbox_cmd,
+ mbox_cmd.fn_arg);
+ return 0;
+ }
+
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_RESPONSE_RECEIVED) {
+ memcpy(&mbox_cmd, &mbox->mbox_resp,
+ sizeof(struct octeon_mbox_cmd));
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ mbox_cmd.recv_status = 0;
+ if (mbox_cmd.fn)
+ mbox_cmd.fn(mbox->oct_dev, &mbox_cmd, mbox_cmd.fn_arg);
+ return 0;
+ }
+
+ if (mbox->state & OCTEON_MBOX_STATE_REQUEST_RECEIVED) {
+ memcpy(&mbox_cmd, &mbox->mbox_req,
+ sizeof(struct octeon_mbox_cmd));
+ if (!mbox_cmd.msg.s.resp_needed) {
+ mbox->state &= ~OCTEON_MBOX_STATE_REQUEST_RECEIVED;
+ if (!(mbox->state &&
+ OCTEON_MBOX_STATE_RESPONSE_PENDING))
+ mbox->state = OCTEON_MBOX_STATE_IDLE;
+ writeq(OCTEON_PFVFSIG, mbox->mbox_read_reg);
+ }
+
+ spin_unlock_irqrestore(&mbox->lock, flags);
+ octeon_mbox_process_cmd(mbox, &mbox_cmd);
+ return 0;
+ }
+
+ WARN_ON(1);
+
+ return 0;
+}
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
new file mode 100644
index 0000000..377a43f
--- /dev/null
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_mailbox.h
@@ -0,0 +1,115 @@
+/**********************************************************************
+ * Author: Cavium, Inc.
+ *
+ * Contact: support@cavium.com
+ * Please include "LiquidIO" in the subject.
+ *
+ * Copyright (c) 2003-2016 Cavium, Inc.
+ *
+ * This file is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License, Version 2, as
+ * published by the Free Software Foundation.
+ *
+ * This file is distributed in the hope that it will be useful, but
+ * AS-IS and WITHOUT ANY WARRANTY; without even the implied warranty
+ * of MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE, TITLE, or
+ * NONINFRINGEMENT. See the GNU General Public License for more details.
+ *
+ * This file may also be available under a different license from Cavium.
+ * Contact Cavium, Inc. for more information
+ ***********************************************************************/
+#ifndef __MAILBOX_H__
+#define __MAILBOX_H__
+
+/* Macros for Mail Box Communication */
+
+#define OCTEON_MBOX_DATA_MAX 32
+
+#define OCTEON_VF_ACTIVE 0x1
+#define OCTEON_VF_FLR_REQUEST 0x2
+#define OCTEON_PF_CHANGED_VF_MACADDR 0x4
+
+/*Macro for Read acknowldgement*/
+#define OCTEON_PFVFACK 0xffffffffffffffff
+#define OCTEON_PFVFSIG 0x1122334455667788
+#define OCTEON_PFVFERR 0xDEADDEADDEADDEAD
+
+enum octeon_mbox_cmd_status {
+ OCTEON_MBOX_STATUS_SUCCESS = 0,
+ OCTEON_MBOX_STATUS_FAILED = 1,
+ OCTEON_MBOX_STATUS_BUSY = 2
+};
+
+enum octeon_mbox_message_type {
+ OCTEON_MBOX_REQUEST = 0,
+ OCTEON_MBOX_RESPONSE = 1
+};
+
+union octeon_mbox_message {
+ u64 u64;
+ struct {
+ u16 type : 1;
+ u16 resp_needed : 1;
+ u16 cmd : 6;
+ u16 len : 8;
+ u8 params[6];
+ } s;
+};
+
+typedef void (*octeon_mbox_callback_t)(void *, void *, void *);
+
+struct octeon_mbox_cmd {
+ union octeon_mbox_message msg;
+ u64 data[OCTEON_MBOX_DATA_MAX];
+ u32 q_no;
+ u32 recv_len;
+ u32 recv_status;
+ octeon_mbox_callback_t fn;
+ void *fn_arg;
+};
+
+enum octeon_mbox_state {
+ OCTEON_MBOX_STATE_IDLE = 1,
+ OCTEON_MBOX_STATE_REQUEST_RECEIVING = 2,
+ OCTEON_MBOX_STATE_REQUEST_RECEIVED = 4,
+ OCTEON_MBOX_STATE_RESPONSE_PENDING = 8,
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVING = 16,
+ OCTEON_MBOX_STATE_RESPONSE_RECEIVED = 16,
+ OCTEON_MBOX_STATE_ERROR = 32
+};
+
+struct octeon_mbox {
+ /** A spinlock to protect access to this q_mbox. */
+ spinlock_t lock;
+
+ struct octeon_device *oct_dev;
+
+ u32 q_no;
+
+ enum octeon_mbox_state state;
+
+ struct cavium_wk mbox_poll_wk;
+
+ /** SLI_MAC_PF_MBOX_INT for PF, SLI_PKT_MBOX_INT for VF. */
+ void *mbox_int_reg;
+
+ /** SLI_PKT_PF_VF_MBOX_SIG(0) for PF, SLI_PKT_PF_VF_MBOX_SIG(1) for VF.
+ */
+ void *mbox_write_reg;
+
+ /** SLI_PKT_PF_VF_MBOX_SIG(1) for PF, SLI_PKT_PF_VF_MBOX_SIG(0) for VF.
+ */
+ void *mbox_read_reg;
+
+ struct octeon_mbox_cmd mbox_req;
+
+ struct octeon_mbox_cmd mbox_resp;
+
+};
+
+int octeon_mbox_read(struct octeon_mbox *mbox);
+int octeon_mbox_write(struct octeon_device *oct,
+ struct octeon_mbox_cmd *mbox_cmd);
+int octeon_mbox_process_message(struct octeon_mbox *mbox);
+
+#endif
diff --git a/drivers/net/ethernet/cavium/liquidio/octeon_main.h b/drivers/net/ethernet/cavium/liquidio/octeon_main.h
index 366298f..3cddf28 100644
--- a/drivers/net/ethernet/cavium/liquidio/octeon_main.h
+++ b/drivers/net/ethernet/cavium/liquidio/octeon_main.h
@@ -66,7 +66,7 @@ void octeon_update_tx_completion_counters(void *buf, int reqtype,
unsigned int *bytes_compl);
void octeon_report_tx_completion_to_bql(void *txq, unsigned int pkts_compl,
unsigned int bytes_compl);
-
+void octeon_pf_changed_vf_macaddr(struct octeon_device *oct, u8 *mac);
/** Swap 8B blocks */
static inline void octeon_swap_8B_data(u64 *data, u32 blocks)
{
--
1.8.3.1
^ permalink raw reply related
* [PATCH (net.git)] stmmac: display the descriptors if DES0 = 0
From: Giuseppe Cavallaro @ 2016-10-20 8:01 UTC (permalink / raw)
To: netdev; +Cc: Giuseppe Cavallaro, Alexandre TORGUE
It makes sense to display the descriptors even if
DES0 is zero. This helps for example in case of it
is needed to dump rx write-back descriptors to get
timestamp status.
Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
Cc: Alexandre TORGUE <alexandre.torgue@st.com>
---
drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
index 4ec7397..a1b17cd 100644
--- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
+++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
@@ -347,10 +347,9 @@ static void dwmac4_display_ring(void *head, unsigned int size, bool rx)
pr_info("%s descriptor ring:\n", rx ? "RX" : "TX");
for (i = 0; i < size; i++) {
- if (p->des0)
- pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
- i, (unsigned int)virt_to_phys(p),
- p->des0, p->des1, p->des2, p->des3);
+ pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
+ i, (unsigned int)virt_to_phys(p),
+ p->des0, p->des1, p->des2, p->des3);
p++;
}
}
--
2.7.4
^ permalink raw reply related
* [PATCH] net: fec: drop check for clk==NULL before calling clk_*
From: Uwe Kleine-König @ 2016-10-20 8:28 UTC (permalink / raw)
To: Fugang Duan; +Cc: kernel, netdev
clk_prepare, clk_enable and their counterparts (at least the common clk
ones, but also most others) do check for the clk being NULL anyhow (and
return 0 then), so there is no gain when the caller checks, too.
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++--------------
1 file changed, 11 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/freescale/fec_main.c b/drivers/net/ethernet/freescale/fec_main.c
index 48a033e64423..1722ccac18c2 100644
--- a/drivers/net/ethernet/freescale/fec_main.c
+++ b/drivers/net/ethernet/freescale/fec_main.c
@@ -1841,11 +1841,11 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
ret = clk_prepare_enable(fep->clk_ahb);
if (ret)
return ret;
- if (fep->clk_enet_out) {
- ret = clk_prepare_enable(fep->clk_enet_out);
- if (ret)
- goto failed_clk_enet_out;
- }
+
+ ret = clk_prepare_enable(fep->clk_enet_out);
+ if (ret)
+ goto failed_clk_enet_out;
+
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
ret = clk_prepare_enable(fep->clk_ptp);
@@ -1857,23 +1857,20 @@ static int fec_enet_clk_enable(struct net_device *ndev, bool enable)
}
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref) {
- ret = clk_prepare_enable(fep->clk_ref);
- if (ret)
- goto failed_clk_ref;
- }
+
+ ret = clk_prepare_enable(fep->clk_ref);
+ if (ret)
+ goto failed_clk_ref;
} else {
clk_disable_unprepare(fep->clk_ahb);
- if (fep->clk_enet_out)
- clk_disable_unprepare(fep->clk_enet_out);
+ clk_disable_unprepare(fep->clk_enet_out);
if (fep->clk_ptp) {
mutex_lock(&fep->ptp_clk_mutex);
clk_disable_unprepare(fep->clk_ptp);
fep->ptp_clk_on = false;
mutex_unlock(&fep->ptp_clk_mutex);
}
- if (fep->clk_ref)
- clk_disable_unprepare(fep->clk_ref);
+ clk_disable_unprepare(fep->clk_ref);
}
return 0;
--
2.9.3
^ permalink raw reply related
* RE: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
From: Stephen Hemminger @ 2016-10-20 8:36 UTC (permalink / raw)
To: Vitaly Kuznetsov, netdev@vger.kernel.org
Cc: devel@linuxdriverproject.org, Haiyang Zhang,
linux-kernel@vger.kernel.org
In-Reply-To: <1476885181-3456-1-git-send-email-vkuznets@redhat.com>
Do we need ACCESS_ONCE() here to avoid check/use issues?
-----Original Message-----
From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
Sent: Wednesday, October 19, 2016 2:53 PM
To: netdev@vger.kernel.org
Cc: Stephen Hemminger <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>
Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
chn_table") turns out to be incomplete. A crash in
netvsc_get_next_send_section() is observed on mtu change when the device is under load. The race I identified is: if we get to netvsc_send() after we set net_device_ctx->nvdev link in netvsc_device_add() but before we finish netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and we crash. Unfortunately we can't set net_device_ctx->nvdev link after the netvsc_init_buf() call as during the negotiation we need to receive packets and on the receive path we check for it. It would probably be possible to split nvdev into a pair of nvdev_in and nvdev_out links and check them accordingly in get_outbound_net_device()/
get_inbound_net_device() but this looks like an overkill.
Check that send_section_map is allocated in netvsc_send().
Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
---
drivers/net/hyperv/netvsc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 720b5fa..e2bfaac 100644
--- a/drivers/net/hyperv/netvsc.c
+++ b/drivers/net/hyperv/netvsc.c
@@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
if (!net_device)
return -ENODEV;
+ /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
+ * here before the negotiation with the host is finished and
+ * send_section_map may not be allocated yet.
+ */
+ if (!net_device->send_section_map)
+ return -EAGAIN;
+
out_channel = net_device->chn_table[q_idx];
packet->send_buf_index = NETVSC_INVALID_INDEX;
--
2.7.4
^ permalink raw reply
* Re: [PATCH net] conntrack: perform a full scan in gc
From: Nicolas Dichtel @ 2016-10-20 8:50 UTC (permalink / raw)
To: Florian Westphal; +Cc: pablo, davem, netdev, netfilter-devel
In-Reply-To: <34d607cc-5556-89c7-673d-be8cb0a4758a@6wind.com>
Le 18/10/2016 à 12:06, Nicolas Dichtel a écrit :
> Le 18/10/2016 à 10:47, Florian Westphal a écrit :
>> Nicolas Dichtel <nicolas.dichtel@6wind.com> wrote:
>>> After commit b87a2f9199ea ("netfilter: conntrack: add gc worker to remove
>>> timed-out entries"), netlink conntrack deletion events may be sent with a
>>> huge delay (5 minutes).
>>>
>>> There is two ways to evict conntrack:
>>> - during a conntrack lookup;
>>> - during a conntrack dump.
>>> Let's do a full scan of conntrack entries after a period of inactivity
>>> (no conntrack lookup).
>>>
>>> CC: Florian Westphal <fw@strlen.de>
>>> Signed-off-by: Nicolas Dichtel <nicolas.dichtel@6wind.com>
>>> ---
>>>
>>> Here is another proposal to try to fix the problem.
>>> Comments are welcomed,
>>> Nicolas
>>
>> Hmm, I don't think its good idea in practice.
>> If goal is to avoid starving arbitrary 'dead' ct for too long,
>> then simple ping will defeat the logic here, because...
>>
>>> net/netfilter/nf_conntrack_core.c | 11 +++++++++--
>>> 1 file changed, 9 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/net/netfilter/nf_conntrack_core.c b/net/netfilter/nf_conntrack_core.c
>>> index ba6a1d421222..3dbb27bd9582 100644
>>> --- a/net/netfilter/nf_conntrack_core.c
>>> +++ b/net/netfilter/nf_conntrack_core.c
>>> @@ -87,6 +87,7 @@ static __read_mostly bool nf_conntrack_locks_all;
>>> #define GC_MAX_BUCKETS 8192u
>>> #define GC_INTERVAL (5 * HZ)
>>> #define GC_MAX_EVICTS 256u
>>> +static bool gc_full_scan = true;
>>>
>>> static struct conntrack_gc_work conntrack_gc_work;
>>>
>>> @@ -511,6 +512,7 @@ ____nf_conntrack_find(struct net *net, const struct nf_conntrack_zone *zone,
>>> unsigned int bucket, hsize;
>>>
>>> begin:
>>> + gc_full_scan = false;
>>
>> ... we do periodic lookup (but always in same slot), so no full scan is
>> triggered.
> Yes, I was wondering about that. My first idea was to have that bool per bucket
> and force a scan of the bucket instead of the whole table.
FYI, I'am off for about two weeks, but we really need to find a way to fix that
before the release goes out.
^ permalink raw reply
* Re: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
From: Vitaly Kuznetsov @ 2016-10-20 8:51 UTC (permalink / raw)
To: Stephen Hemminger
Cc: netdev@vger.kernel.org, Haiyang Zhang,
linux-kernel@vger.kernel.org, devel@linuxdriverproject.org
In-Reply-To: <BLUPR0301MB2098D6A07F0D465E63988D9FCCD50@BLUPR0301MB2098.namprd03.prod.outlook.com>
Stephen Hemminger <sthemmin@microsoft.com> writes:
> Do we need ACCESS_ONCE() here to avoid check/use issues?
>
I think we don't: this is the only place in the function where we read
the variable so we'll get normal read. We're not trying to syncronize
with netvsc_init_buf() as that would require locking, if we read stale
NULL value after it was already updated on a different CPU we're fine,
we'll just return -EAGAIN.
> -----Original Message-----
> From: Vitaly Kuznetsov [mailto:vkuznets@redhat.com]
> Sent: Wednesday, October 19, 2016 2:53 PM
> To: netdev@vger.kernel.org
> Cc: Stephen Hemminger <sthemmin@microsoft.com>; devel@linuxdriverproject.org; linux-kernel@vger.kernel.org; KY Srinivasan <kys@microsoft.com>; Haiyang Zhang <haiyangz@microsoft.com>
> Subject: [PATCH net-next] hv_netvsc: fix a race between netvsc_send() and netvsc_init_buf()
>
> Fix in commit 880988348270 ("hv_netvsc: set nvdev link after populating
> chn_table") turns out to be incomplete. A crash in
> netvsc_get_next_send_section() is observed on mtu change when the device is under load. The race I identified is: if we get to netvsc_send() after we set net_device_ctx->nvdev link in netvsc_device_add() but before we finish netvsc_connect_vsp()->netvsc_init_buf() send_section_map is not allocated and we crash. Unfortunately we can't set net_device_ctx->nvdev link after the netvsc_init_buf() call as during the negotiation we need to receive packets and on the receive path we check for it. It would probably be possible to split nvdev into a pair of nvdev_in and nvdev_out links and check them accordingly in get_outbound_net_device()/
> get_inbound_net_device() but this looks like an overkill.
>
> Check that send_section_map is allocated in netvsc_send().
>
> Signed-off-by: Vitaly Kuznetsov <vkuznets@redhat.com>
> ---
> drivers/net/hyperv/netvsc.c | 7 +++++++
> 1 file changed, 7 insertions(+)
>
> diff --git a/drivers/net/hyperv/netvsc.c b/drivers/net/hyperv/netvsc.c index 720b5fa..e2bfaac 100644
> --- a/drivers/net/hyperv/netvsc.c
> +++ b/drivers/net/hyperv/netvsc.c
> @@ -888,6 +888,13 @@ int netvsc_send(struct hv_device *device,
> if (!net_device)
> return -ENODEV;
>
> + /* We may race with netvsc_connect_vsp()/netvsc_init_buf() and get
> + * here before the negotiation with the host is finished and
> + * send_section_map may not be allocated yet.
> + */
> + if (!net_device->send_section_map)
> + return -EAGAIN;
> +
> out_channel = net_device->chn_table[q_idx];
>
> packet->send_buf_index = NETVSC_INVALID_INDEX;
> --
> 2.7.4
--
Vitaly
^ permalink raw reply
* [PATCH] bnx2x: Replace semaphore stats_lock with mutex
From: Binoy Jayan @ 2016-10-20 8:52 UTC (permalink / raw)
To: Ariel Elior; +Cc: Arnd Bergmann, netdev, linux-kernel, Binoy Jayan
stats_lock is used as a simple mutex, so replace it with a mutex.
Semaphores are going away in the future.
Signed-off-by: Binoy Jayan <binoy.jayan@linaro.org>
---
They following is a patch which removes semaphores from bnx2x.
Its part of a bigger effort to eliminate all semaphores
from the linux kernel.
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 3 ++-
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 +++---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 17 +++++++----------
3 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 7dd7490..fd5e5b8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -25,6 +25,7 @@
#include <linux/ptp_clock_kernel.h>
#include <linux/net_tstamp.h>
#include <linux/timecounter.h>
+#include <linux/mutex.h>
/* compilation time flags */
@@ -1698,7 +1699,7 @@ struct bnx2x {
int stats_state;
/* used for synchronization of concurrent threads statistics handling */
- struct semaphore stats_lock;
+ struct mutex stats_lock;
/* used by dmae command loader */
struct dmae_command stats_dmae;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 20fe6a8..b323d69 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12340,7 +12340,7 @@ static int bnx2x_init_bp(struct bnx2x *bp)
mutex_init(&bp->port.phy_mutex);
mutex_init(&bp->fw_mb_mutex);
mutex_init(&bp->drv_info_mutex);
- sema_init(&bp->stats_lock, 1);
+ mutex_init(&bp->stats_lock);
bp->drv_info_mng_owner = false;
INIT_LIST_HEAD(&bp->vlan_reg);
@@ -14205,9 +14205,9 @@ static int bnx2x_eeh_nic_unload(struct bnx2x *bp)
cancel_delayed_work_sync(&bp->sp_task);
cancel_delayed_work_sync(&bp->period_task);
- if (!down_timeout(&bp->stats_lock, HZ / 10)) {
+ if (mutex_trylock(&bp->stats_lock)) {
bp->stats_state = STATS_STATE_DISABLED;
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
}
bnx2x_save_statistics(bp);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
index 7e0919a..ee6ffd8 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
@@ -1374,23 +1374,20 @@ void bnx2x_stats_handle(struct bnx2x *bp, enum bnx2x_stats_event event)
* that context in case someone is in the middle of a transition.
* For other events, wait a bit until lock is taken.
*/
- if (down_trylock(&bp->stats_lock)) {
+ if (!mutex_trylock(&bp->stats_lock)) {
if (event == STATS_EVENT_UPDATE)
return;
DP(BNX2X_MSG_STATS,
"Unlikely stats' lock contention [event %d]\n", event);
- if (unlikely(down_timeout(&bp->stats_lock, HZ / 10))) {
- BNX2X_ERR("Failed to take stats lock [event %d]\n",
- event);
- return;
- }
+ BNX2X_ERR("Failed to take stats lock [event %d]\n", event);
+ return;
}
bnx2x_stats_stm[state][event].action(bp);
bp->stats_state = bnx2x_stats_stm[state][event].next_state;
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
if ((event != STATS_EVENT_UPDATE) || netif_msg_timer(bp))
DP(BNX2X_MSG_STATS, "state %d -> event %d -> state %d\n",
@@ -1976,8 +1973,8 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
/* Wait for statistics to end [while blocking further requests],
* then run supplied function 'safely'.
*/
- rc = down_timeout(&bp->stats_lock, HZ / 10);
- if (unlikely(rc)) {
+ rc = mutex_trylock(&bp->stats_lock);
+ if (unlikely(!rc)) {
BNX2X_ERR("Failed to take statistics lock for safe execution\n");
goto out_no_lock;
}
@@ -1998,7 +1995,7 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
/* No need to restart statistics - if they're enabled, the timer
* will restart the statistics.
*/
- up(&bp->stats_lock);
+ mutex_unlock(&bp->stats_lock);
out_no_lock:
return rc;
}
--
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-10-20 8:41 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <20161019100253.vxqxp5fhoxrlt6ay@linux-x5ow.site>
Thanks Johannes for the review, please see comments below,
On 19/10/16 3:32 PM, "Johannes Thumshirn" <jthumshirn@suse.de> wrote:
>On Wed, Oct 19, 2016 at 01:01:10AM -0400, manish.rangankar@cavium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>
>[...]
>
>> +static inline void *qedi_get_task_mem(struct qed_iscsi_tid *info, u32
>>tid)
>> +{
>> + return (void *)(info->blocks[tid / info->num_tids_per_block] +
>> + (tid % info->num_tids_per_block) * info->size);
>> +}
>
>Unnecessary cast here.
Noted
>
>
>[...]
>
>> +void
>> +qedi_dbg_err(struct qedi_dbg_ctx *qedi, const char *func, u32 line,
>> + const char *fmt, ...)
>> +{
>> + va_list va;
>> + struct va_format vaf;
>> + char nfunc[32];
>> +
>> + memset(nfunc, 0, sizeof(nfunc));
>> + memcpy(nfunc, func, sizeof(nfunc) - 1);
>> +
>> + va_start(va, fmt);
>> +
>> + vaf.fmt = fmt;
>> + vaf.va = &va;
>> +
>> + if (likely(qedi) && likely(qedi->pdev))
>> + pr_crit("[%s]:[%s:%d]:%d: %pV", dev_name(&qedi->pdev->dev),
>> + nfunc, line, qedi->host_no, &vaf);
>> + else
>> + pr_crit("[0000:00:00.0]:[%s:%d]: %pV", nfunc, line, &vaf);
>
>pr_crit, seriously?
We will change it to pr_err.
>
>[...]
>
>> +static void qedi_int_fp(struct qedi_ctx *qedi)
>> +{
>> + struct qedi_fastpath *fp;
>> + int id;
>> +
>> + memset((void *)qedi->fp_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->fp_array));
>> + memset((void *)qedi->sb_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->sb_array));
>
>I don't think the cast is necessary here.
Noted
>
>[...]
>
>> +static int qedi_setup_cid_que(struct qedi_ctx *qedi)
>> +{
>> + int i;
>> +
>> + qedi->cid_que.cid_que_base = kmalloc((qedi->max_active_conns *
>> + sizeof(u32)), GFP_KERNEL);
>> + if (!qedi->cid_que.cid_que_base)
>> + return -ENOMEM;
>> +
>> + qedi->cid_que.conn_cid_tbl = kmalloc((qedi->max_active_conns *
>> + sizeof(struct qedi_conn *)),
>> + GFP_KERNEL);
>
>Please use kmalloc_array() here.
Will do.
>
>[...]
>
>> +/* MSI-X fastpath handler code */
>> +static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
>> +{
>> + struct qedi_fastpath *fp = dev_id;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + bool wake_io_thread = true;
>> +
>> + qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
>> +
>> +process_again:
>> + wake_io_thread = qedi_process_completions(fp);
>> + if (wake_io_thread) {
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
>> + "process already running\n");
>> + }
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_update_sb_idx(fp->sb_info);
>> +
>> + /* Check for more work */
>> + rmb();
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
>> + else
>> + goto process_again;
>> +
>> + return IRQ_HANDLED;
>> +}
>
>You might want to consider workqueues here.
We will revisit this code.
>
>[...]
>
>> +static int qedi_alloc_itt(struct qedi_ctx *qedi)
>> +{
>> + qedi->itt_map = kzalloc((sizeof(struct qedi_itt_map) *
>> + MAX_ISCSI_TASK_ENTRIES), GFP_KERNEL);
>
>that screams for kcalloc()
>
>> + if (!qedi->itt_map) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to allocate itt map array memory\n");
>> + return -ENOMEM;
>> + }
>> + return 0;
>> +}
>> +
>> +static void qedi_free_itt(struct qedi_ctx *qedi)
>> +{
>> + kfree(qedi->itt_map);
>> +}
>> +
>> +static struct qed_ll2_cb_ops qedi_ll2_cb_ops = {
>> + .rx_cb = qedi_ll2_rx,
>> + .tx_cb = NULL,
>> +};
>> +
>> +static int qedi_percpu_io_thread(void *arg)
>> +{
>> + struct qedi_percpu_s *p = arg;
>> + struct qedi_work *work, *tmp;
>> + unsigned long flags;
>> + LIST_HEAD(work_list);
>> +
>> + set_user_nice(current, -20);
>> +
>> + while (!kthread_should_stop()) {
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + while (!list_empty(&p->work_list)) {
>> + list_splice_init(&p->work_list, &work_list);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> +
>> + list_for_each_entry_safe(work, tmp, &work_list, list) {
>> + list_del_init(&work->list);
>> + qedi_fp_process_cqes(work->qedi, &work->cqe,
>> + work->que_idx);
>> + kfree(work);
>> + }
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + }
>> + set_current_state(TASK_INTERRUPTIBLE);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> + schedule();
>> + }
>> + __set_current_state(TASK_RUNNING);
>> +
>> + return 0;
>> +}
>
>A kthread with prio -20 IRQs turned off looping over a list, what could
>possibly go wrong here. I bet you your favorite beverage that this will
>cause Soft Lockups when running I/O stress tests BTDT.
Will remove this.
>
>[...]
>
>> + if (mode != QEDI_MODE_RECOVERY) {
>> + if (iscsi_host_add(qedi->shost, &pdev->dev)) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not add iscsi host\n");
>> + rc = -ENOMEM;
>> + goto remove_host;
>> + }
>> +
>> + /* Allocate uio buffers */
>> + rc = qedi_alloc_uio_rings(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "UIO alloc ring failed err=%d\n", rc);
>> + goto remove_host;
>> + }
>> +
>> + rc = qedi_init_uio(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "UIO init failed, err=%d\n", rc);
>> + goto free_uio;
>> + }
>> +
>> + /* host the array on iscsi_conn */
>> + rc = qedi_setup_cid_que(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not setup cid que\n");
>> + goto free_uio;
>> + }
>> +
>> + rc = qedi_cm_alloc_mem(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not alloc cm memory\n");
>> + goto free_cid_que;
>> + }
>> +
>> + rc = qedi_alloc_itt(qedi);
>> + if (rc) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Could not alloc itt memory\n");
>> + goto free_cid_que;
>> + }
>> +
>> + sprintf(host_buf, "host_%d", qedi->shost->host_no);
>> + qedi->tmf_thread = create_singlethread_workqueue(host_buf);
>> + if (!qedi->tmf_thread) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to start tmf thread!\n");
>> + rc = -ENODEV;
>> + goto free_cid_que;
>> + }
>> +
>> + sprintf(host_buf, "qedi_ofld%d", qedi->shost->host_no);
>> + qedi->offload_thread = create_workqueue(host_buf);
>> + if (!qedi->offload_thread) {
>> + QEDI_ERR(&qedi->dbg_ctx,
>> + "Unable to start offload thread!\n");
>> + rc = -ENODEV;
>> + goto free_cid_que;
>> + }
>> +
>> + /* F/w needs 1st task context memory entry for performance */
>> + set_bit(QEDI_RESERVE_TASK_ID, qedi->task_idx_map);
>> + atomic_set(&qedi->num_offloads, 0);
>> + }
>> +
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_INFO,
>> + "QLogic FastLinQ iSCSI Module qedi %s, FW %d.%d.%d.%d\n",
>> + QEDI_MODULE_VERSION, FW_MAJOR_VERSION, FW_MINOR_VERSION,
>> + FW_REVISION_VERSION, FW_ENGINEERING_VERSION);
>> + return 0;
>
>Please put the QEDI_INFO() above the if and invert the condition.
Will do.
^ permalink raw reply
* RE: [PATCH] net: fec: drop check for clk==NULL before calling clk_*
From: Andy Duan @ 2016-10-20 8:56 UTC (permalink / raw)
To: Uwe Kleine-König; +Cc: kernel@pengutronix.de, netdev@vger.kernel.org
In-Reply-To: <20161020082827.598-1-u.kleine-koenig@pengutronix.de>
From: Uwe Kleine-König <u.kleine-koenig@pengutronix.de> Sent: Thursday, October 20, 2016 4:28 PM
> To: Andy Duan <fugang.duan@nxp.com>
> Cc: kernel@pengutronix.de; netdev@vger.kernel.org
> Subject: [PATCH] net: fec: drop check for clk==NULL before calling clk_*
>
> clk_prepare, clk_enable and their counterparts (at least the common clk
> ones, but also most others) do check for the clk being NULL anyhow (and
> return 0 then), so there is no gain when the caller checks, too.
>
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> ---
Acked-by: Fugang Duan <fugang.duan@nxp.com>
> drivers/net/ethernet/freescale/fec_main.c | 25 +++++++++++--------------
> 1 file changed, 11 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/fec_main.c
> b/drivers/net/ethernet/freescale/fec_main.c
> index 48a033e64423..1722ccac18c2 100644
> --- a/drivers/net/ethernet/freescale/fec_main.c
> +++ b/drivers/net/ethernet/freescale/fec_main.c
> @@ -1841,11 +1841,11 @@ static int fec_enet_clk_enable(struct net_device
> *ndev, bool enable)
> ret = clk_prepare_enable(fep->clk_ahb);
> if (ret)
> return ret;
> - if (fep->clk_enet_out) {
> - ret = clk_prepare_enable(fep->clk_enet_out);
> - if (ret)
> - goto failed_clk_enet_out;
> - }
> +
> + ret = clk_prepare_enable(fep->clk_enet_out);
> + if (ret)
> + goto failed_clk_enet_out;
> +
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> ret = clk_prepare_enable(fep->clk_ptp); @@ -
> 1857,23 +1857,20 @@ static int fec_enet_clk_enable(struct net_device
> *ndev, bool enable)
> }
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref) {
> - ret = clk_prepare_enable(fep->clk_ref);
> - if (ret)
> - goto failed_clk_ref;
> - }
> +
> + ret = clk_prepare_enable(fep->clk_ref);
> + if (ret)
> + goto failed_clk_ref;
> } else {
> clk_disable_unprepare(fep->clk_ahb);
> - if (fep->clk_enet_out)
> - clk_disable_unprepare(fep->clk_enet_out);
> + clk_disable_unprepare(fep->clk_enet_out);
> if (fep->clk_ptp) {
> mutex_lock(&fep->ptp_clk_mutex);
> clk_disable_unprepare(fep->clk_ptp);
> fep->ptp_clk_on = false;
> mutex_unlock(&fep->ptp_clk_mutex);
> }
> - if (fep->clk_ref)
> - clk_disable_unprepare(fep->clk_ref);
> + clk_disable_unprepare(fep->clk_ref);
> }
>
> return 0;
> --
> 2.9.3
^ permalink raw reply
* Re: [RFC 3/6] qedi: Add QLogic FastLinQ offload iSCSI driver framework.
From: Rangankar, Manish @ 2016-10-20 8:27 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <50c821e3-28dc-78b1-8fa4-527a138f1709@suse.de>
Thanks Hannes for the review, please see my comments below,
On 19/10/16 1:15 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> The QLogic FastLinQ Driver for iSCSI (qedi) is the iSCSI specific module
>> for 41000 Series Converged Network Adapters by QLogic.
>>
>> This patch consists of following changes:
>> - MAINTAINERS Makefile and Kconfig changes for qedi,
>> - PCI driver registration,
>> - iSCSI host level initialization,
>> - Debugfs and log level infrastructure.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> MAINTAINERS | 6 +
>> drivers/net/ethernet/qlogic/Kconfig | 12 -
>> drivers/scsi/Kconfig | 1 +
>> drivers/scsi/Makefile | 1 +
>> drivers/scsi/qedi/Kconfig | 10 +
>> drivers/scsi/qedi/Makefile | 5 +
>> drivers/scsi/qedi/qedi.h | 286 +++++++
>> drivers/scsi/qedi/qedi_dbg.c | 143 ++++
>> drivers/scsi/qedi/qedi_dbg.h | 144 ++++
>> drivers/scsi/qedi/qedi_debugfs.c | 244 ++++++
>> drivers/scsi/qedi/qedi_hsi.h | 52 ++
>> drivers/scsi/qedi/qedi_main.c | 1550
>>+++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_sysfs.c | 52 ++
>> drivers/scsi/qedi/qedi_version.h | 14 +
>> 14 files changed, 2508 insertions(+), 12 deletions(-)
>> create mode 100644 drivers/scsi/qedi/Kconfig
>> create mode 100644 drivers/scsi/qedi/Makefile
>> create mode 100644 drivers/scsi/qedi/qedi.h
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.c
>> create mode 100644 drivers/scsi/qedi/qedi_dbg.h
>> create mode 100644 drivers/scsi/qedi/qedi_debugfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_hsi.h
>> create mode 100644 drivers/scsi/qedi/qedi_main.c
>> create mode 100644 drivers/scsi/qedi/qedi_sysfs.c
>> create mode 100644 drivers/scsi/qedi/qedi_version.h
>>
>> diff --git a/MAINTAINERS b/MAINTAINERS
>> index 5e925a2..906d05f 100644
>> --- a/MAINTAINERS
>> +++ b/MAINTAINERS
>> @@ -9909,6 +9909,12 @@ F: drivers/net/ethernet/qlogic/qed/
>> F: include/linux/qed/
>> F: drivers/net/ethernet/qlogic/qede/
>>
>> +QLOGIC QL41xxx ISCSI DRIVER
>> +M: QLogic-Storage-Upstream@cavium.com
>> +L: linux-scsi@vger.kernel.org
>> +S: Supported
>> +F: drivers/scsi/qedi/
>> +
>> QNX4 FILESYSTEM
>> M: Anders Larsen <al@alarsen.net>
>> W: http://www.alarsen.net/linux/qnx4fs/
>> diff --git a/drivers/net/ethernet/qlogic/Kconfig
>>b/drivers/net/ethernet/qlogic/Kconfig
>> index bad4fae..28b4366 100644
>> --- a/drivers/net/ethernet/qlogic/Kconfig
>> +++ b/drivers/net/ethernet/qlogic/Kconfig
>> @@ -121,16 +121,4 @@ config INFINIBAND_QEDR
>> config QED_ISCSI
>> bool
>>
>> -config QEDI
>> - tristate "QLogic QED 25/40/100Gb iSCSI driver"
>> - depends on QED
>> - select QED_LL2
>> - select QED_ISCSI
>> - default n
>> - ---help---
>> - This provides a temporary node that allows the compilation
>> - and logical testing of the hardware offload iSCSI support
>> - for QLogic QED. This would be replaced by the 'real' option
>> - once the QEDI driver is added [+relocated].
>> -
>> endif # NET_VENDOR_QLOGIC
>Huh? You just introduce this one in patch 1/6.
>Please fold them together so that this can be omitted.
Yes, we will remove this in the next revision.
-- snipped --
>> @@ -0,0 +1,52 @@
>> +/*
>> + * QLogic iSCSI Offload Driver
>> + * Copyright (c) 2016 Cavium Inc.
>> + *
>> + * This software is available under the terms of the GNU General
>>Public License
>> + * (GPL) Version 2, available from the file COPYING in the main
>>directory of
>> + * this source tree.
>> + */
>> +#ifndef __QEDI_HSI__
>> +#define __QEDI_HSI__
>> +/********************************/
>> +/* Add include to common target */
>> +/********************************/
>> +#include <linux/qed/common_hsi.h>
>> +
>Please use kernel-doc style for comments
Will do.
--snipped--
>> +static void qedi_int_fp(struct qedi_ctx *qedi)
>> +{
>> + struct qedi_fastpath *fp;
>> + int id;
>> +
>> + memset((void *)qedi->fp_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->fp_array));
>> + memset((void *)qedi->sb_array, 0, MIN_NUM_CPUS_MSIX(qedi) *
>> + sizeof(*qedi->sb_array));
>> +
>> + for (id = 0; id < MIN_NUM_CPUS_MSIX(qedi); id++) {
>> + fp = &qedi->fp_array[id];
>> + fp->sb_info = &qedi->sb_array[id];
>> + fp->sb_id = id;
>> + fp->qedi = qedi;
>> + snprintf(fp->name, sizeof(fp->name), "%s-fp-%d",
>> + "qedi", id);
>> +
>> + /* fp_array[i] ---- irq cookie
>> + * So init data which is needed in int ctx
>> + */
>> + }
>> +}
>> +
>Please check if you cannot make use of Christophs irq rework.
Sure, we will explore this.
--snipped--
>> +static bool qedi_process_completions(struct qedi_fastpath *fp)
>> +{
>> + struct qedi_work *qedi_work = NULL;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + struct qed_sb_info *sb_info = fp->sb_info;
>> + struct status_block *sb = sb_info->sb_virt;
>> + struct qedi_percpu_s *p = NULL;
>> + struct global_queue *que;
>> + u16 prod_idx;
>> + unsigned long flags;
>> + union iscsi_cqe *cqe;
>> + int cpu;
>> +
>> + /* Get the current firmware producer index */
>> + prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
>> +
>> + if (prod_idx >= QEDI_CQ_SIZE)
>> + prod_idx = prod_idx % QEDI_CQ_SIZE;
>> +
>> + que = qedi->global_queues[fp->sb_id];
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
>> + "Before: global queue=%p prod_idx=%d cons_idx=%d, sb_id=%d\n",
>> + que, prod_idx, que->cq_cons_idx, fp->sb_id);
>> +
>> + qedi->intr_cpu = fp->sb_id;
>> + cpu = smp_processor_id();
>> + p = &per_cpu(qedi_percpu, cpu);
>> +
>> + if (unlikely(!p->iothread))
>> + WARN_ON(1);
>> +
>> + spin_lock_irqsave(&p->p_work_lock, flags);
>> + while (que->cq_cons_idx != prod_idx) {
>> + cqe = &que->cq[que->cq_cons_idx];
>> +
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_IO,
>> + "cqe=%p prod_idx=%d cons_idx=%d.\n",
>> + cqe, prod_idx, que->cq_cons_idx);
>> +
>> + /* Alloc and copy to the cqe */
>> + qedi_work = kzalloc(sizeof(*qedi_work), GFP_ATOMIC);
>> + if (qedi_work) {
>> + INIT_LIST_HEAD(&qedi_work->list);
>> + qedi_work->qedi = qedi;
>> + memcpy(&qedi_work->cqe, cqe, sizeof(union iscsi_cqe));
>> + qedi_work->que_idx = fp->sb_id;
>> + list_add_tail(&qedi_work->list, &p->work_list);
>> + } else {
>> + WARN_ON(1);
>> + continue;
>> + }
>> +
>Memory allocation in an interrupt routine?
>You must be kidding ...
We will revisit this code.
>
>> + que->cq_cons_idx++;
>> + if (que->cq_cons_idx == QEDI_CQ_SIZE)
>> + que->cq_cons_idx = 0;
>> + }
>> + wake_up_process(p->iothread);
>> + spin_unlock_irqrestore(&p->p_work_lock, flags);
>> +
>> + return true;
>> +}
>> +
>> +static bool qedi_fp_has_work(struct qedi_fastpath *fp)
>> +{
>> + struct qedi_ctx *qedi = fp->qedi;
>> + struct global_queue *que;
>> + struct qed_sb_info *sb_info = fp->sb_info;
>> + struct status_block *sb = sb_info->sb_virt;
>> + u16 prod_idx;
>> +
>> + barrier();
>> +
>> + /* Get the current firmware producer index */
>> + prod_idx = sb->pi_array[QEDI_PROTO_CQ_PROD_IDX];
>> +
>> + /* Get the pointer to the global CQ this completion is on */
>> + que = qedi->global_queues[fp->sb_id];
>> +
>> + /* prod idx wrap around uint16 */
>> + if (prod_idx >= QEDI_CQ_SIZE)
>> + prod_idx = prod_idx % QEDI_CQ_SIZE;
>> +
>> + return (que->cq_cons_idx != prod_idx);
>> +}
>> +
>> +/* MSI-X fastpath handler code */
>> +static irqreturn_t qedi_msix_handler(int irq, void *dev_id)
>> +{
>> + struct qedi_fastpath *fp = dev_id;
>> + struct qedi_ctx *qedi = fp->qedi;
>> + bool wake_io_thread = true;
>> +
>> + qed_sb_ack(fp->sb_info, IGU_INT_DISABLE, 0);
>> +
>> +process_again:
>> + wake_io_thread = qedi_process_completions(fp);
>> + if (wake_io_thread) {
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_DISC,
>> + "process already running\n");
>> + }
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_update_sb_idx(fp->sb_info);
>> +
>> + /* Check for more work */
>> + rmb();
>> +
>> + if (qedi_fp_has_work(fp) == 0)
>> + qed_sb_ack(fp->sb_info, IGU_INT_ENABLE, 1);
>> + else
>> + goto process_again;
>> +
>> + return IRQ_HANDLED;
>> +}
>> +
>> +/* simd handler for MSI/INTa */
>> +static void qedi_simd_int_handler(void *cookie)
>> +{
>> + /* Cookie is qedi_ctx struct */
>> + struct qedi_ctx *qedi = (struct qedi_ctx *)cookie;
>> +
>> + QEDI_WARN(&qedi->dbg_ctx, "qedi=%p.\n", qedi);
>> +}
>> +
>> +#define QEDI_SIMD_HANDLER_NUM 0
>> +static void qedi_sync_free_irqs(struct qedi_ctx *qedi)
>> +{
>> + int i;
>> +
>> + if (qedi->int_info.msix_cnt) {
>> + for (i = 0; i < qedi->int_info.used_cnt; i++) {
>> + synchronize_irq(qedi->int_info.msix[i].vector);
>> + irq_set_affinity_hint(qedi->int_info.msix[i].vector,
>> + NULL);
>> + free_irq(qedi->int_info.msix[i].vector,
>> + &qedi->fp_array[i]);
>> + }
>> + } else {
>> + qedi_ops->common->simd_handler_clean(qedi->cdev,
>> + QEDI_SIMD_HANDLER_NUM);
>> + }
>> +
>> + qedi->int_info.used_cnt = 0;
>> + qedi_ops->common->set_fp_int(qedi->cdev, 0);
>> +}
>> +
>Again, consider using the interrupt affinity rework from Christoph Hellwig
Sure, we will explore this one also.
^ permalink raw reply
* Re: [RFC 5/6] qedi: Add support for iSCSI session management.
From: Rangankar, Manish @ 2016-10-20 9:09 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <6f79e92c-ea12-331b-5c20-e9a3587b1d80@suse.de>
On 19/10/16 1:33 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> drivers/scsi/qedi/qedi_fw.c | 1123 ++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_gbl.h | 67 ++
>> drivers/scsi/qedi/qedi_iscsi.c | 1604
>>++++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_iscsi.h | 228 ++++++
>> drivers/scsi/qedi/qedi_main.c | 164 ++++
>> 5 files changed, 3186 insertions(+)
>> create mode 100644 drivers/scsi/qedi/qedi_fw.c
>> create mode 100644 drivers/scsi/qedi/qedi_gbl.h
>> create mode 100644 drivers/scsi/qedi/qedi_iscsi.c
>> create mode 100644 drivers/scsi/qedi/qedi_iscsi.h
>>
--snipped--
>>
>> +static void qedi_process_async_mesg(struct qedi_ctx *qedi,
>> + union iscsi_cqe *cqe,
>> + struct iscsi_task *task,
>> + struct qedi_conn *qedi_conn,
>> + u16 que_idx)
>> +{
>> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> + struct iscsi_session *session = conn->session;
>> + struct iscsi_async_msg_hdr *cqe_async_msg;
>> + struct iscsi_async *resp_hdr;
>> + u32 scsi_lun[2];
>> + u32 pdu_len, num_bdqs;
>> + char bdq_data[QEDI_BDQ_BUF_SIZE];
>> + unsigned long flags;
>> +
>> + spin_lock_bh(&session->back_lock);
>> +
>> + cqe_async_msg = &cqe->cqe_common.iscsi_hdr.async_msg;
>> + pdu_len = cqe_async_msg->hdr_second_dword &
>> + ISCSI_ASYNC_MSG_HDR_DATA_SEG_LEN_MASK;
>> + num_bdqs = pdu_len / QEDI_BDQ_BUF_SIZE;
>> +
>> + if (cqe->cqe_common.cqe_type == ISCSI_CQE_TYPE_UNSOLICITED) {
>> + spin_lock_irqsave(&qedi->hba_lock, flags);
>> + qedi_unsol_pdu_adjust_bdq(qedi, &cqe->cqe_unsolicited,
>> + pdu_len, num_bdqs, bdq_data);
>> + spin_unlock_irqrestore(&qedi->hba_lock, flags);
>> + }
>> +
>> + resp_hdr = (struct iscsi_async *)&qedi_conn->gen_pdu.resp_hdr;
>> + memset(resp_hdr, 0, sizeof(struct iscsi_hdr));
>> + resp_hdr->opcode = cqe_async_msg->opcode;
>> + resp_hdr->flags = 0x80;
>> +
>> + scsi_lun[0] = cpu_to_be32(cqe_async_msg->lun.lo);
>> + scsi_lun[1] = cpu_to_be32(cqe_async_msg->lun.hi);
>I _think_ we have a SCSI LUN structure ...
Will do.
--snipped--
>> +void qedi_process_iscsi_error(struct qedi_endpoint *ep, struct
>>async_data *data)
>> +{
>> + struct qedi_conn *qedi_conn;
>> + struct qedi_ctx *qedi;
>> + char warn_notice[] = "iscsi_warning";
>> + char error_notice[] = "iscsi_error";
>> + char *message;
>> + int need_recovery = 0;
>> + u32 err_mask = 0;
>> + char msg[64];
>> +
>> + if (!ep)
>> + return;
>> +
>> + qedi_conn = ep->conn;
>> + if (!qedi_conn)
>> + return;
>> +
>> + qedi = ep->qedi;
>> +
>> + QEDI_ERR(&qedi->dbg_ctx, "async event iscsi error:0x%x\n",
>> + data->error_code);
>> +
>> + if (err_mask) {
>> + need_recovery = 0;
>> + message = warn_notice;
>> + } else {
>> + need_recovery = 1;
>> + message = error_notice;
>> + }
>> +
>> + switch (data->error_code) {
>> + case ISCSI_STATUS_NONE:
>> + strcpy(msg, "tcp_error none");
>> + break;
>> + case ISCSI_CONN_ERROR_TASK_CID_MISMATCH:
>> + strcpy(msg, "task cid mismatch");
>> + break;
>> + case ISCSI_CONN_ERROR_TASK_NOT_VALID:
>> + strcpy(msg, "invalid task");
>> + break;
>> + case ISCSI_CONN_ERROR_RQ_RING_IS_FULL:
>> + strcpy(msg, "rq ring full");
>> + break;
>> + case ISCSI_CONN_ERROR_CMDQ_RING_IS_FULL:
>> + strcpy(msg, "cmdq ring full");
>> + break;
>> + case ISCSI_CONN_ERROR_HQE_CACHING_FAILED:
>> + strcpy(msg, "sge caching failed");
>> + break;
>> + case ISCSI_CONN_ERROR_HEADER_DIGEST_ERROR:
>> + strcpy(msg, "hdr digest error");
>> + break;
>> + case ISCSI_CONN_ERROR_LOCAL_COMPLETION_ERROR:
>> + strcpy(msg, "local cmpl error");
>> + break;
>> + case ISCSI_CONN_ERROR_DATA_OVERRUN:
>> + strcpy(msg, "invalid task");
>> + break;
>> + case ISCSI_CONN_ERROR_OUT_OF_SGES_ERROR:
>> + strcpy(msg, "out of sge error");
>> + break;
>> + case ISCSI_CONN_ERROR_TCP_SEG_PROC_IP_OPTIONS_ERROR:
>> + strcpy(msg, "tcp seg ip options error");
>> + break;
>> + case ISCSI_CONN_ERROR_TCP_IP_FRAGMENT_ERROR:
>> + strcpy(msg, "tcp ip fragment error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_AHS_LEN:
>> + strcpy(msg, "AHS len protocol error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_ITT_OUT_OF_RANGE:
>> + strcpy(msg, "itt out of range error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SEG_LEN_EXCEEDS_PDU_SIZE:
>> + strcpy(msg, "data seg more than pdu size");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_OPCODE:
>> + strcpy(msg, "invalid opcode");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_OPCODE_BEFORE_UPDATE:
>> + strcpy(msg, "invalid opcode before update");
>> + break;
>> + case ISCSI_CONN_ERROR_UNVALID_NOPIN_DSL:
>> + strcpy(msg, "unexpected opcode");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_CARRIES_NO_DATA:
>> + strcpy(msg, "r2t carries no data");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SN:
>> + strcpy(msg, "data sn error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_IN_TTT:
>> + strcpy(msg, "data TTT error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_TTT:
>> + strcpy(msg, "r2t TTT error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_BUFFER_OFFSET:
>> + strcpy(msg, "buffer offset error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_BUFFER_OFFSET_OOO:
>> + strcpy(msg, "buffer offset ooo");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_R2T_SN:
>> + strcpy(msg, "data seg len 0");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_0:
>> + strcpy(msg, "data xer len error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_1:
>> + strcpy(msg, "data xer len1 error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DESIRED_DATA_TRNS_LEN_2:
>> + strcpy(msg, "data xer len2 error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_LUN:
>> + strcpy(msg, "protocol lun error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_F_BIT_ZERO:
>> + strcpy(msg, "f bit zero error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_F_BIT_ZERO_S_BIT_ONE:
>> + strcpy(msg, "f bit zero s bit one error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_EXP_STAT_SN:
>> + strcpy(msg, "exp stat sn error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DSL_NOT_ZERO:
>> + strcpy(msg, "dsl not zero error");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_INVALID_DSL:
>> + strcpy(msg, "invalid dsl");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_DATA_SEG_LEN_TOO_BIG:
>> + strcpy(msg, "data seg len too big");
>> + break;
>> + case ISCSI_CONN_ERROR_PROTOCOL_ERR_OUTSTANDING_R2T_COUNT:
>> + strcpy(msg, "outstanding r2t count error");
>> + break;
>> + case ISCSI_CONN_ERROR_SENSE_DATA_LENGTH:
>> + strcpy(msg, "sense datalen error");
>> + break;
>Please use an array for mapping values onto strings.
Will add this change in next revision.
Thanks,
Manish R.
^ permalink raw reply
* Re: [RFC 5/6] qedi: Add support for iSCSI session management.
From: Rangankar, Manish @ 2016-10-20 9:12 UTC (permalink / raw)
To: Johannes Thumshirn
Cc: lduncan@suse.com, cleech@redhat.com, martin.petersen@oracle.com,
jejb@linux.vnet.ibm.com, linux-scsi@vger.kernel.org,
netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <20161019132801.eptbf2dpuoilplpd@linux-x5ow.site>
On 19/10/16 6:58 PM, "Johannes Thumshirn" <jthumshirn@suse.de> wrote:
>On Wed, Oct 19, 2016 at 01:01:12AM -0400, manish.rangankar@cavium.com
>wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for iscsi_transport LLD Login,
>> Logout, NOP-IN/NOP-OUT, Async, Reject PDU processing
>> and Firmware async event handling support.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>
>[...]
>
>> +void qedi_iscsi_unmap_sg_list(struct qedi_cmd *cmd)
>> +{
>> + struct scsi_cmnd *sc = cmd->scsi_cmd;
>> +
>> + if (cmd->io_tbl.sge_valid && sc) {
>> + scsi_dma_unmap(sc);
>> + cmd->io_tbl.sge_valid = 0;
>> + }
>> +}
>
>Maybe set sge_valid to 0 and then call scsi_dma_unmap(). I don't know if
>it's
>really racy but it looks like it is.
>
>[...]
>
>> +static void qedi_process_text_resp(struct qedi_ctx *qedi,
>> + union iscsi_cqe *cqe,
>> + struct iscsi_task *task,
>> + struct qedi_conn *qedi_conn)
>> +{
>> + struct iscsi_conn *conn = qedi_conn->cls_conn->dd_data;
>> + struct iscsi_session *session = conn->session;
>> + struct iscsi_task_context *task_ctx;
>> + struct iscsi_text_rsp *resp_hdr_ptr;
>> + struct iscsi_text_response_hdr *cqe_text_response;
>> + struct qedi_cmd *cmd;
>> + int pld_len;
>> + u32 *tmp;
>> +
>> + cmd = (struct qedi_cmd *)task->dd_data;
>> + task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(&qedi->tasks,
>> + cmd->task_id);
>
>No need to cast here, qedi_get_task_mem() returns void *.
>
>[...]
>
>> + cqe_login_response = &cqe->cqe_common.iscsi_hdr.login_response;
>> + task_ctx = (struct iscsi_task_context
>>*)qedi_get_task_mem(&qedi->tasks,
>> + cmd->task_id);
>
>Same here.
>
>[...]
>
>> + }
>> +
>> + pbl = (struct scsi_bd *)qedi->bdq_pbl;
>> + pbl += (qedi->bdq_prod_idx % qedi->rq_num_entries);
>> + pbl->address.hi =
>> + cpu_to_le32((u32)(((u64)(qedi->bdq[idx].buf_dma)) >> 32));
>> + pbl->address.lo =
>> + cpu_to_le32(((u32)(((u64)(qedi->bdq[idx].buf_dma)) &
>> + 0xffffffff)));
>
>Is this LISP or C?
>
>> + QEDI_INFO(&qedi->dbg_ctx, QEDI_LOG_CONN,
>> + "pbl [0x%p] pbl->address hi [0x%llx] lo [0x%llx] idx [%d]\n",
>> + pbl, pbl->address.hi, pbl->address.lo, idx);
>> + pbl->opaque.hi = cpu_to_le32((u32)(((u64)0) >> 32));
>
>Isn't this plain pbl->opaque.hi = 0; ?
>
>> + pbl->opaque.lo = cpu_to_le32(((u32)(((u64)idx) & 0xffffffff)));
>> +
>
>[...]
>
>> + switch (comp_type) {
>> + case ISCSI_CQE_TYPE_SOLICITED:
>> + case ISCSI_CQE_TYPE_SOLICITED_WITH_SENSE:
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>> + cqe->cqe_solicited.itid);
>
>Again, no cast needed.
>
>[...]
>
>> + writel(*(u32 *)&dbell, qedi_conn->ep->p_doorbell);
>> + /* Make sure fw idx is coherent */
>> + wmb();
>> + mmiowb();
>
>Isn't either wmb() or mmiowb() enough?
>
>[..]
>
>> +
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>
>Cast again.
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>
>^^
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks, tid);
>
>
>[...]
>
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>> +
>
>[...]
>
>> +
>> + qedi = (struct qedi_ctx *)iscsi_host_priv(shost);
>
>Same goes for iscsi_host_priv();
>
>[...]
>
>> + ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
>> + ((qedi_ep->state ==
>> + EP_STATE_OFLDCONN_FAILED) ||
>> + (qedi_ep->state ==
>> + EP_STATE_OFLDCONN_COMPL)),
>> + msecs_to_jiffies(timeout_ms));
>
>Maybe:
>#define QEDI_OLDCON_STATE(q) ((q)->state == EP_STATE_OFLDCONN_FAILED || \
> (q)->state == EP_STATE_OFLDCONN_COMPL)
>
>ret = wait_event_interruptible_timeout(qedi_ep->ofld_wait,
> QEDI_OLDCON_STATE(qedi_ep),
> msec_to_jiffies(timeout_ms));
>
>But that could be just me hating linewraps.
>
>[...]
We will address all the above review comments in the next revision.
Thanks,
Manish R.
^ permalink raw reply
* Re: [PATCH] bnx2x: Replace semaphore stats_lock with mutex
From: Arnd Bergmann @ 2016-10-20 9:16 UTC (permalink / raw)
To: Binoy Jayan; +Cc: Ariel Elior, netdev, linux-kernel
In-Reply-To: <1476953532-2019-1-git-send-email-binoy.jayan@linaro.org>
On Thursday, October 20, 2016 2:22:12 PM CEST Binoy Jayan wrote:
> @@ -1976,8 +1973,8 @@ int bnx2x_stats_safe_exec(struct bnx2x *bp,
> /* Wait for statistics to end [while blocking further requests],
> * then run supplied function 'safely'.
> */
> - rc = down_timeout(&bp->stats_lock, HZ / 10);
> - if (unlikely(rc)) {
> + rc = mutex_trylock(&bp->stats_lock);
> + if (unlikely(!rc)) {
> BNX2X_ERR("Failed to take statistics lock for safe execution\n");
> goto out_no_lock;
> }
This conversion looks suspicious, your changelog text does not mention
at all why would be to remove the timeout and fail immediately.
In fact, you don't seem to have any mutex_lock() call left, just
mutex_trylock(), and everything that tries to get the mutex fails
immediately if someone else holds it. The existing behavior of
the driver is not much better (it always gives up after 100ms),
but I think this needs some more thought put into it.
Arnd
^ permalink raw reply
* Re: [PATCH (net.git)] stmmac: display the descriptors if DES0 = 0
From: Alexandre Torgue @ 2016-10-20 9:29 UTC (permalink / raw)
To: Giuseppe Cavallaro, netdev
In-Reply-To: <1476950488-24923-1-git-send-email-peppe.cavallaro@st.com>
Hi Peppe,
On 10/20/2016 10:01 AM, Giuseppe Cavallaro wrote:
> It makes sense to display the descriptors even if
> DES0 is zero. This helps for example in case of it
> is needed to dump rx write-back descriptors to get
> timestamp status.
>
> Signed-off-by: Giuseppe Cavallaro <peppe.cavallaro@st.com>
> Cc: Alexandre TORGUE <alexandre.torgue@st.com>
> ---
> drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c | 7 +++----
> 1 file changed, 3 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> index 4ec7397..a1b17cd 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/dwmac4_descs.c
> @@ -347,10 +347,9 @@ static void dwmac4_display_ring(void *head, unsigned int size, bool rx)
> pr_info("%s descriptor ring:\n", rx ? "RX" : "TX");
>
> for (i = 0; i < size; i++) {
> - if (p->des0)
> - pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
> - i, (unsigned int)virt_to_phys(p),
> - p->des0, p->des1, p->des2, p->des3);
> + pr_info("%d [0x%x]: 0x%x 0x%x 0x%x 0x%x\n",
> + i, (unsigned int)virt_to_phys(p),
> + p->des0, p->des1, p->des2, p->des3);
> p++;
> }
> }
>
I agree. Acked-by: Alexandre Torgue <alexandre.torgue@st.com>
Thanks
Alex
^ permalink raw reply
* Re: [PATCH net] tipc: Guard against tiny MTU in tipc_msg_build()
From: Ying Xue @ 2016-10-20 9:30 UTC (permalink / raw)
To: Ben Hutchings, Jon Maloy; +Cc: netdev, Qian Zhang, Eric Dumazet
In-Reply-To: <20161019021654.GD2773@decadent.org.uk>
On 10/19/2016 10:16 AM, Ben Hutchings wrote:
> Qian Zhang (张谦) reported a potential socket buffer overflow in
> tipc_msg_build(). The minimum fragment length needs to be checked
> against the maximum packet size, which is based on the link MTU.
>
> Reported-by: Qian Zhang (张谦) <zhangqian-c@360.cn>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
> This is untested, but I think it fixes the issue reported. Ideally
> tipc_l2_device_event() would also disable use of TIPC on devices with
> too small an MTU, like several other protocols do.
>
Yes, I think so. I will create a patch to disable TIPC sending process
when MTU size is too small.
> Ben.
>
> net/tipc/msg.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/net/tipc/msg.c b/net/tipc/msg.c
> index 17201aa8423d..b9124ac82c29 100644
> --- a/net/tipc/msg.c
> +++ b/net/tipc/msg.c
> @@ -274,6 +274,10 @@ int tipc_msg_build(struct tipc_msg *mhdr, struct msghdr *m,
> goto error;
> }
>
> + /* Check that fragment and message header will fit */
> + if (INT_H_SIZE + mhsz > pktmax)
> + return -EMSGSIZE;
The "mhsz" represents the size of tipc packet header for current socket,
INT_H_SIZE indicates the size of tipc internal message header. So it
seems unreasonable to identify whether the sum of both header sizes is
bigger than MTU size. In my opinion, it's better to use MAX_H_SIZE to
compare it with pktmax. If MAX_H_SIZE is bigger than pktmax, we should
return EMSGSIZE error code.
> +
> /* Prepare reusable fragment header */
> tipc_msg_init(msg_prevnode(mhdr), &pkthdr, MSG_FRAGMENTER,
> FIRST_FRAGMENT, INT_H_SIZE, msg_destnode(mhdr));
>
^ permalink raw reply
* Re: [RFC 6/6] qedi: Add support for data path.
From: Rangankar, Manish @ 2016-10-20 9:24 UTC (permalink / raw)
To: Hannes Reinecke, lduncan@suse.com, cleech@redhat.com
Cc: martin.petersen@oracle.com, jejb@linux.vnet.ibm.com,
linux-scsi@vger.kernel.org, netdev@vger.kernel.org, Mintz, Yuval,
Dept-Eng QLogic Storage Upstream, Javali, Nilesh,
Adheer Chandravanshi, Dupuis, Chad, Kashyap, Saurav, Easi, Arun
In-Reply-To: <fc58b63a-ac67-59e4-7698-4ab742848080@suse.de>
On 19/10/16 3:54 PM, "Hannes Reinecke" <hare@suse.de> wrote:
>On 10/19/2016 07:01 AM, manish.rangankar@cavium.com wrote:
>> From: Manish Rangankar <manish.rangankar@cavium.com>
>>
>> This patch adds support for data path and TMF handling.
>>
>> Signed-off-by: Nilesh Javali <nilesh.javali@cavium.com>
>> Signed-off-by: Adheer Chandravanshi <adheer.chandravanshi@qlogic.com>
>> Signed-off-by: Chad Dupuis <chad.dupuis@cavium.com>
>> Signed-off-by: Saurav Kashyap <saurav.kashyap@cavium.com>
>> Signed-off-by: Arun Easi <arun.easi@cavium.com>
>> Signed-off-by: Manish Rangankar <manish.rangankar@cavium.com>
>> ---
>> drivers/scsi/qedi/qedi_fw.c | 1282
>>++++++++++++++++++++++++++++++++++++++++
>> drivers/scsi/qedi/qedi_gbl.h | 6 +
>> drivers/scsi/qedi/qedi_iscsi.c | 6 +
>> drivers/scsi/qedi/qedi_main.c | 4 +
>> 4 files changed, 1298 insertions(+)
>>
>> diff --git a/drivers/scsi/qedi/qedi_fw.c b/drivers/scsi/qedi/qedi_fw.c
>> index a820785..af1e14d 100644
>> --- a/drivers/scsi/qedi/qedi_fw.c
>> +++ b/drivers/scsi/qedi/qedi_fw.c
>> @@ -147,6 +147,114 @@ static void qedi_process_text_resp(struct
>>qedi_ctx *qedi,
>> spin_unlock(&session->back_lock);
>> }
--snipped--
>> +void qedi_trace_io(struct qedi_ctx *qedi, struct iscsi_task *task,
>> + u16 tid, int8_t direction)
>> +{
>> + struct qedi_io_log *io_log;
>> + struct iscsi_conn *conn = task->conn;
>> + struct qedi_conn *qedi_conn = conn->dd_data;
>> + struct scsi_cmnd *sc_cmd = task->sc;
>> + unsigned long flags;
>> + u8 op;
>> +
>> + spin_lock_irqsave(&qedi->io_trace_lock, flags);
>> +
>> + io_log = &qedi->io_trace_buf[qedi->io_trace_idx];
>> + io_log->direction = direction;
>> + io_log->task_id = tid;
>> + io_log->cid = qedi_conn->iscsi_conn_id;
>> + io_log->lun = sc_cmd->device->lun;
>> + io_log->op = sc_cmd->cmnd[0];
>> + op = sc_cmd->cmnd[0];
>> +
>> + if (op == READ_10 || op == WRITE_10) {
>> + io_log->lba[0] = sc_cmd->cmnd[2];
>> + io_log->lba[1] = sc_cmd->cmnd[3];
>> + io_log->lba[2] = sc_cmd->cmnd[4];
>> + io_log->lba[3] = sc_cmd->cmnd[5];
>> + } else {
>> + io_log->lba[0] = 0;
>> + io_log->lba[1] = 0;
>> + io_log->lba[2] = 0;
>> + io_log->lba[3] = 0;
>> + }
>Only for READ_10 and WRITE_10? What about the other read or write
>commands?
We will add support for other scsi commands in the next revision.
>
>> + io_log->bufflen = scsi_bufflen(sc_cmd);
>> + io_log->sg_count = scsi_sg_count(sc_cmd);
>> + io_log->fast_sgs = qedi->fast_sgls;
>> + io_log->cached_sgs = qedi->cached_sgls;
>> + io_log->slow_sgs = qedi->slow_sgls;
>> + io_log->cached_sge = qedi->use_cached_sge;
>> + io_log->slow_sge = qedi->use_slow_sge;
>> + io_log->fast_sge = qedi->use_fast_sge;
>> + io_log->result = sc_cmd->result;
>> + io_log->jiffies = jiffies;
>> + io_log->blk_req_cpu = smp_processor_id();
>> +
>> + if (direction == QEDI_IO_TRACE_REQ) {
>> + /* For requests we only care about the submission CPU */
>> + io_log->req_cpu = smp_processor_id() % qedi->num_queues;
>> + io_log->intr_cpu = 0;
>> + io_log->blk_rsp_cpu = 0;
>> + } else if (direction == QEDI_IO_TRACE_RSP) {
>> + io_log->req_cpu = smp_processor_id() % qedi->num_queues;
>> + io_log->intr_cpu = qedi->intr_cpu;
>> + io_log->blk_rsp_cpu = smp_processor_id();
>> + }
>> +
>> + qedi->io_trace_idx++;
>> + if (qedi->io_trace_idx == QEDI_IO_TRACE_SIZE)
>> + qedi->io_trace_idx = 0;
>> +
>> + qedi->use_cached_sge = false;
>> + qedi->use_slow_sge = false;
>> + qedi->use_fast_sge = false;
>> +
>> + spin_unlock_irqrestore(&qedi->io_trace_lock, flags);
>> +}
>> +
>> +int qedi_iscsi_send_ioreq(struct iscsi_task *task)
>> +{
>> + struct iscsi_conn *conn = task->conn;
>> + struct iscsi_session *session = conn->session;
>> + struct Scsi_Host *shost =
>>iscsi_session_to_shost(session->cls_session);
>> + struct qedi_ctx *qedi = iscsi_host_priv(shost);
>> + struct qedi_conn *qedi_conn = conn->dd_data;
>> + struct qedi_cmd *cmd = task->dd_data;
>> + struct scsi_cmnd *sc = task->sc;
>> + struct iscsi_task_context *fw_task_ctx;
>> + struct iscsi_cached_sge_ctx *cached_sge;
>> + struct iscsi_phys_sgl_ctx *phys_sgl;
>> + struct iscsi_virt_sgl_ctx *virt_sgl;
>> + struct ystorm_iscsi_task_st_ctx *yst_cxt;
>> + struct mstorm_iscsi_task_st_ctx *mst_cxt;
>> + struct iscsi_sgl *sgl_struct;
>> + struct iscsi_sge *single_sge;
>> + struct iscsi_scsi_req *hdr = (struct iscsi_scsi_req *)task->hdr;
>> + struct iscsi_sge *bd = cmd->io_tbl.sge_tbl;
>> + enum iscsi_task_type task_type;
>> + struct iscsi_cmd_hdr *fw_cmd;
>> + u32 scsi_lun[2];
>> + u16 cq_idx = smp_processor_id() % qedi->num_queues;
>> + s16 ptu_invalidate = 0;
>> + s16 tid = 0;
>> + u8 num_fast_sgs;
>> +
>> + tid = qedi_get_task_idx(qedi);
>> + if (tid == -1)
>> + return -ENOMEM;
>> +
>> + qedi_iscsi_map_sg_list(cmd);
>> +
>> + int_to_scsilun(sc->device->lun, (struct scsi_lun *)scsi_lun);
>> + fw_task_ctx =
>> + (struct iscsi_task_context *)qedi_get_task_mem(&qedi->tasks,
>>tid);
>> +
>> + memset(fw_task_ctx, 0, sizeof(struct iscsi_task_context));
>> + cmd->task_id = tid;
>> +
>> + /* Ystrom context */
>Ystrom or Ystorm?
Noted
>
>> + fw_cmd = &fw_task_ctx->ystorm_st_context.pdu_hdr.cmd;
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_ATTR, ISCSI_ATTR_SIMPLE);
>> +
>> + if (sc->sc_data_direction == DMA_TO_DEVICE) {
>> + if (conn->session->initial_r2t_en) {
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min((conn->session->imm_data_en *
>> + conn->max_xmit_dlength),
>> + conn->session->first_burst);
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min(fw_task_ctx->ustorm_ag_context.exp_data_acked,
>> + scsi_bufflen(sc));
>> + } else {
>> + fw_task_ctx->ustorm_ag_context.exp_data_acked =
>> + min(conn->session->first_burst, scsi_bufflen(sc));
>> + }
>> +
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_WRITE, 1);
>> + task_type = ISCSI_TASK_TYPE_INITIATOR_WRITE;
>> + } else {
>> + if (scsi_bufflen(sc))
>> + SET_FIELD(fw_cmd->flags_attr, ISCSI_CMD_HDR_READ, 1);
>> + task_type = ISCSI_TASK_TYPE_INITIATOR_READ;
>> + }
>> +
>> + fw_cmd->lun.lo = be32_to_cpu(scsi_lun[0]);
>> + fw_cmd->lun.hi = be32_to_cpu(scsi_lun[1]);
>> +
>> + qedi_update_itt_map(qedi, tid, task->itt);
>> + fw_cmd->itt = qedi_set_itt(tid, get_itt(task->itt));
>> + fw_cmd->expected_transfer_length = scsi_bufflen(sc);
>> + fw_cmd->cmd_sn = be32_to_cpu(hdr->cmdsn);
>> + fw_cmd->opcode = hdr->opcode;
>> + qedi_cpy_scsi_cdb(sc, (u32 *)fw_cmd->cdb);
>> +
>> + /* Mstorm context */
>> + fw_task_ctx->mstorm_st_context.sense_db.lo =
>>(u32)cmd->sense_buffer_dma;
>> + fw_task_ctx->mstorm_st_context.sense_db.hi =
>> + (u32)((u64)cmd->sense_buffer_dma >> 32);
>> + fw_task_ctx->mstorm_ag_context.task_cid = qedi_conn->iscsi_conn_id;
>> + fw_task_ctx->mstorm_st_context.task_type = task_type;
>> +
>> + if (qedi->tid_reuse_count[tid] == QEDI_MAX_TASK_NUM) {
>> + ptu_invalidate = 1;
>> + qedi->tid_reuse_count[tid] = 0;
>> + }
>> + fw_task_ctx->ystorm_st_context.state.reuse_count =
>> + qedi->tid_reuse_count[tid];
>> + fw_task_ctx->mstorm_st_context.reuse_count =
>> + qedi->tid_reuse_count[tid]++;
>> +
>> + /* Ustrorm context */
>Ustrorm?
Noted
Thanks,
Manish R.
^ permalink raw reply
* Re: [PATCH 0/4] make POSIX timers optional with some Kconfig help
From: Thomas Gleixner @ 2016-10-20 9:46 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Josh Triplett, netdev,
linux-kbuild, linux-kernel
In-Reply-To: <1476920573-14384-1-git-send-email-nicolas.pitre@linaro.org>
On Wed, 19 Oct 2016, Nicolas Pitre wrote:
> Therefore this series also includes kconfig changes to implement a new
> keyword to express some reverse dependencies like "select" does, named
> "imply", and still allowing for the target config symbol to be disabled
> if the user or a direct dependency says so.
That's really nice work! Thanks for doing that. It makes the whole thing
more palatable.
Thanks,
tglx
^ permalink raw reply
* Re: [PATCH 3/4] ptp_clock: allow for it to be optional
From: Thomas Gleixner @ 2016-10-20 9:50 UTC (permalink / raw)
To: Nicolas Pitre
Cc: John Stultz, Richard Cochran, Yann E MORIN, Josh Triplett, netdev,
linux-kbuild, linux-kernel
In-Reply-To: <1476920573-14384-4-git-send-email-nicolas.pitre@linaro.org>
On Wed, 19 Oct 2016, Nicolas Pitre wrote:
> The pch_gbe driver is a bit special as it relies on extra code in
> drivers/ptp/ptp_pch.c. Therefore we let the make process descend into
> drivers/ptp/ even if PTP_1588_CLOCK is unselected.
The above paragraph looks like a leftover of the previous patch set.
Thanks,
tglx
^ permalink raw reply
* Re: [Patch net] net: saving irq context for peernet2id()
From: Nicolas Dichtel @ 2016-10-20 10:17 UTC (permalink / raw)
To: Cong Wang, netdev; +Cc: sds, e, paul
In-Reply-To: <1476946352-15770-1-git-send-email-xiyou.wangcong@gmail.com>
Le 20/10/2016 à 08:52, Cong Wang a écrit :
> A kernel warning inside __local_bh_enable_ip() was reported by people
> running SELinux, this is caused due to some SELinux functions
> (indirectly) call peernet2id() with IRQ disabled in process context,
> when we re-enable BH with IRQ disabled kernel complains. Shut up this
> warning by saving IRQ context in peernet2id(), BH is still implicitly
> disabled.
Franckly, reverting the original patch seems better for me. The intention for
that patch was "it's not needed", see the commit log:
"We never read or change netns id in hardirq context,
the only place we read netns id in softirq context
is in vxlan_xmit(). So, it should be enough to just
disable BH."
Now, we see that "it's needed" and that the analysis was wrong. If a race is
introduced by this patch, it will be hard to detect and fix it.
Regards,
Nicolas
^ permalink raw reply
* Re: [PATCH net-next v4 2/3] udp: implement memory accounting helpers
From: Paolo Abeni @ 2016-10-20 10:18 UTC (permalink / raw)
To: Eric Dumazet
Cc: netdev-u79uwXL29TY76Z2rM5mHXA, David S. Miller, James Morris,
Trond Myklebust, Alexander Duyck, Daniel Borkmann, Eric Dumazet,
Tom Herbert, Hannes Frederic Sowa, Edward Cree,
linux-nfs-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <1476938622.5650.111.camel-XN9IlZ5yJG9HTL0Zs8A6p+yfmBU6pStAUsxypvmhUTTZJqsBc5GL+g@public.gmane.org>
On Wed, 2016-10-19 at 21:43 -0700, Eric Dumazet wrote:
> On Wed, 2016-10-19 at 14:47 +0200, Paolo Abeni wrote:
> > Avoid using the generic helpers.
> > Adds a new spinlock_t to the udp_sock structure and use
> > it to protect the memory accounting operation, both on
> > enqueue and on dequeue.
> >
> > On dequeue perform partial memory reclaiming, trying to
> > leave a quantum of forward allocated memory.
> >
> > On enqueue use a custom helper, to allow some optimizations:
> > - use a plain spin_lock() variant instead of the slightly
> > costly spin_lock_irqsave(),
> > - avoid dst_force check, since the calling code has already
> > dropped the skb dst
> >
> > The above needs custom memory reclaiming on shutdown, provided
> > by the udp_destruct_sock().
> >
> > v3 -> v4:
> > - reworked memory accunting, simplifying the schema
> > - provide an helper for both memory scheduling and enqueuing
> >
> > v1 -> v2:
> > - use a udp specific destrctor to perform memory reclaiming
> > - remove a couple of helpers, unneeded after the above cleanup
> > - do not reclaim memory on dequeue if not under memory
> > pressure
> > - reworked the fwd accounting schema to avoid potential
> > integer overflow
> >
> > Acked-by: Hannes Frederic Sowa <hannes-tFNcAqjVMyqKXQKiL6tip0B+6BGkLq7r@public.gmane.org>
> > Signed-off-by: Paolo Abeni <pabeni-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > ---
> > include/linux/udp.h | 1 +
> > include/net/udp.h | 4 ++
> > net/ipv4/udp.c | 111 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 116 insertions(+)
> >
> > diff --git a/include/linux/udp.h b/include/linux/udp.h
> > index d1fd8cd..8ea7f8b 100644
> > --- a/include/linux/udp.h
> > +++ b/include/linux/udp.h
> > @@ -66,6 +66,7 @@ struct udp_sock {
> > #define UDPLITE_RECV_CC 0x4 /* set via udplite setsocktopt */
> > __u8 pcflag; /* marks socket as UDP-Lite if > 0 */
> > __u8 unused[3];
> > + spinlock_t mem_lock; /* protects memory accounting */
> > /*
> > * For encapsulation sockets.
> > */
> > diff --git a/include/net/udp.h b/include/net/udp.h
> > index ea53a87..18f1e6b 100644
> > --- a/include/net/udp.h
> > +++ b/include/net/udp.h
> > @@ -246,6 +246,9 @@ static inline __be16 udp_flow_src_port(struct net *net, struct sk_buff *skb,
> > }
> >
> > /* net/ipv4/udp.c */
> > +void skb_consume_udp(struct sock *sk, struct sk_buff *skb, int len);
> > +int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb);
> > +
> > void udp_v4_early_demux(struct sk_buff *skb);
> > int udp_get_port(struct sock *sk, unsigned short snum,
> > int (*saddr_cmp)(const struct sock *,
> > @@ -258,6 +261,7 @@ int udp_get_port(struct sock *sk, unsigned short snum,
> > void udp4_hwcsum(struct sk_buff *skb, __be32 src, __be32 dst);
> > int udp_rcv(struct sk_buff *skb);
> > int udp_ioctl(struct sock *sk, int cmd, unsigned long arg);
> > +int udp_init_sock(struct sock *sk);
> > int udp_disconnect(struct sock *sk, int flags);
> > unsigned int udp_poll(struct file *file, struct socket *sock, poll_table *wait);
> > struct sk_buff *skb_udp_tunnel_segment(struct sk_buff *skb,
> > diff --git a/net/ipv4/udp.c b/net/ipv4/udp.c
> > index 7d96dc2..7047bb3 100644
> > --- a/net/ipv4/udp.c
> > +++ b/net/ipv4/udp.c
> > @@ -1172,6 +1172,117 @@ int udp_sendpage(struct sock *sk, struct page *page, int offset,
> > return ret;
> > }
> >
> > +static void udp_rmem_release(struct sock *sk, int size, int partial)
> > +{
> > + struct udp_sock *up = udp_sk(sk);
> > + int amt;
> > +
> > + atomic_sub(size, &sk->sk_rmem_alloc);
> > +
> > + spin_lock_bh(&up->mem_lock);
> > + sk->sk_forward_alloc += size;
> > + amt = (sk->sk_forward_alloc - partial) & ~(SK_MEM_QUANTUM - 1);
> > + sk->sk_forward_alloc -= amt;
> > + spin_unlock_bh(&up->mem_lock);
> > +
> > + if (amt)
> > + __sk_mem_reduce_allocated(sk, amt >> SK_MEM_QUANTUM_SHIFT);
> > +}
> > +
> > +static void udp_rmem_free(struct sk_buff *skb)
> > +{
> > + udp_rmem_release(skb->sk, skb->truesize, 1);
> > +}
> > +
> > +int __udp_enqueue_schedule_skb(struct sock *sk, struct sk_buff *skb)
> > +{
> > + struct sk_buff_head *list = &sk->sk_receive_queue;
> > + int rmem, delta, amt, err = -ENOMEM;
> > + struct udp_sock *up = udp_sk(sk);
> > + int size = skb->truesize;
> > +
> > + /* try to avoid the costly atomic add/sub pair when the receive
> > + * queue is full
> > + */
> > + if (atomic_read(&sk->sk_rmem_alloc) + size > sk->sk_rcvbuf)
> > + goto drop;
> > +
> > + rmem = atomic_add_return(size, &sk->sk_rmem_alloc);
> > + if (rmem > sk->sk_rcvbuf)
> > + goto uncharge_drop;
>
> This is dangerous : We used to accept one packet in receive queue, even
> if sk_rcvbuf is too small. (Check __sock_queue_rcv_skb())
>
> With your code we might drop all packets even if receive queue is empty,
> this might add regressions for some apps.
>
> > +
> > + spin_lock(&up->mem_lock);
> > + if (size < sk->sk_forward_alloc)
> > + goto unlock;
>
> Strange label name, since you do "sk->sk_forward_alloc -= size;"before
> unlock.
>
> You could avoid the goto by :
>
> if (size >= sk->sk_forward_alloc) {
> amt = sk_mem_pages(size);
> ...
> }
> sk->sk_forward_alloc += delta;k_mem_schedule(
>
> > +
> > + amt = sk_mem_pages(size);
> > + delta = amt << SK_MEM_QUANTUM_SHIFT;
> > + if (!__sk_mem_raise_allocated(sk, delta, amt, SK_MEM_RECV)) {
> > + err = -ENOBUFS;
> > + spin_unlock(&up->mem_lock);
> > + goto uncharge_drop;
> > + }
> > +
> > + sk->sk_forward_alloc += delta;
> > +
> > +unlock:
> > + sk->sk_forward_alloc -= size;
> > + spin_unlock(&up->mem_lock);
> > +
> > + /* the skb owner in now the udp socket */
> > + skb_orphan(skb);
> > + skb->sk = sk;
> > + skb->destructor = udp_rmem_free;
>
> > +
> > + skb->dev = NULL;
> > + sock_skb_set_dropcount(sk, skb);
> > +
> > + spin_lock(&list->lock);
>
> Since we acquire this lock anyway to store the packet,
> why not protecting sk_forward_alloc as well with this spinlock,
> and get rid of up->mem_lock.
>
> Ideally a single spin_lock() should be enough.
I thought that would hit us back, since udp_recvmsg() needs to acquire
it twice and we are increasing the bh lock scope, but a quick test
showed otherwise!!!
> Then almost all other atomic ops could be transformed to non atomic ops.
I'm not sure which atomic operations you refer to ?!? AFAICS we still
need them to manipulate udp_memory_allocated, sk_rmem_alloc, and
sk_drops. Can you please hint which ones ?
Thank you,
Paolo
--
To unsubscribe from this list: send the line "unsubscribe linux-nfs" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply
* Re: [PATCH] ipv6: properly prevent temp_prefered_lft sysctl race
From: Jiri Bohac @ 2016-10-20 10:29 UTC (permalink / raw)
To: David Miller
Cc: julia.lawall, kuznet, jmorris, yoshfuji, kaber, netdev,
kbuild-all
In-Reply-To: <20161019.145816.259194714319437266.davem@davemloft.net>
The check for an underflow of tmp_prefered_lft is always false
because tmp_prefered_lft is unsigned. The intention of the check
was to guard against racing with an update of the
temp_prefered_lft sysctl, potentially resulting in an underflow.
As suggested by David Miller, the best way to prevent the race is
by reading the sysctl variable using READ_ONCE.
Signed-off-by: Jiri Bohac <jbohac@suse.cz>
Reported-by: Julia Lawall <julia.lawall@lip6.fr>
Fixes: 76506a986dc3 ("IPv6: fix DESYNC_FACTOR")
diff --git a/net/ipv6/addrconf.c b/net/ipv6/addrconf.c
index cc7c26d..060dd99 100644
--- a/net/ipv6/addrconf.c
+++ b/net/ipv6/addrconf.c
@@ -1185,6 +1185,7 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
u32 addr_flags;
unsigned long now = jiffies;
long max_desync_factor;
+ s32 cnf_temp_preferred_lft;
write_lock_bh(&idev->lock);
if (ift) {
@@ -1228,9 +1229,10 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
/* recalculate max_desync_factor each time and update
* idev->desync_factor if it's larger
*/
+ cnf_temp_preferred_lft = READ_ONCE(idev->cnf.temp_prefered_lft);
max_desync_factor = min_t(__u32,
idev->cnf.max_desync_factor,
- idev->cnf.temp_prefered_lft - regen_advance);
+ cnf_temp_preferred_lft - regen_advance);
if (unlikely(idev->desync_factor > max_desync_factor)) {
if (max_desync_factor > 0) {
@@ -1245,11 +1247,8 @@ static int ipv6_create_tempaddr(struct inet6_ifaddr *ifp, struct inet6_ifaddr *i
tmp_valid_lft = min_t(__u32,
ifp->valid_lft,
idev->cnf.temp_valid_lft + age);
- tmp_prefered_lft = idev->cnf.temp_prefered_lft + age -
+ tmp_prefered_lft = cnf_temp_preferred_lft + age -
idev->desync_factor;
- /* guard against underflow in case of concurrent updates to cnf */
- if (unlikely(tmp_prefered_lft < 0))
- tmp_prefered_lft = 0;
tmp_prefered_lft = min_t(__u32, ifp->prefered_lft, tmp_prefered_lft);
tmp_plen = ifp->prefix_len;
tmp_tstamp = ifp->tstamp;
--
Jiri Bohac <jbohac@suse.cz>
SUSE Labs, SUSE CZ
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox