* [PATCH 1/4] dt-bindings/marvell-8xxx: Allow wake-up interrupt to be placed in a separate node
From: Marc Zyngier @ 2019-02-24 14:04 UTC (permalink / raw)
To: Amitkumar Karwar, Enric Balletbo i Serra, Ganapathi Bhat,
Heiko Stuebner, Kalle Valo, Nishant Sarmukadam, Rob Herring,
Xinming Hu
Cc: David S. Miller, devicetree, linux-arm-kernel, linux-kernel,
linux-rockchip, linux-wireless, netdev
In-Reply-To: <20190224140426.3267-1-marc.zyngier@arm.com>
The DT binding for the PCI version of the Marvell 8xxx wifi devices
is pretty broken (the fact that a PCI device requires a DT binding
is quite telling on its own).
The binding allows the description of a wake-up interrupt as a sideband
signal, allowing the wifi device to wake-up the system. So far, so good.
Until you realise that placing an interrupt in the PCI device DT node
has a specific meaning, and applies to the actual PCI function, and
not some random sideband stuff. This is of course in total violation
of the OF specification (IEEE Std 1275-1994), but hey, who cares.
Let's thus change the binding to be somewhat compatible with the spec,
by placing the wake-up interrupt in a subnode called "wake-up". This
still is an optional property, but a recommended one for PCI devices.
Signed-off-by: Marc Zyngier <marc.zyngier@arm.com>
---
.../bindings/net/wireless/marvell-8xxx.txt | 23 ++++++++++++++++++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt b/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
index 9bf9bbac16e2..f9340ca37047 100644
--- a/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
+++ b/Documentation/devicetree/bindings/net/wireless/marvell-8xxx.txt
@@ -33,10 +33,16 @@ Optional properties:
this interrupt number. during system suspend, the irq will be enabled
so that the wifi chip can wakeup host platform under certain condition.
during system resume, the irq will be disabled to make sure
- unnecessary interrupt is not received.
+ unnecessary interrupt is not received. For PCI devices, it
+ is recommended that this property is placed in a "wake-up"
+ sub-node in order to limit the ambiguity with PCI legacy
+ interrupts.
- vmmc-supply: a phandle of a regulator, supplying VCC to the card
- mmc-pwrseq: phandle to the MMC power sequence node. See "mmc-pwrseq-*"
for documentation of MMC power sequence bindings.
+ - wake-up: a subnode containing the interrupt specifier for a wake-up
+ interrupt. This is the recommended configuration for PCI
+ devices.
Example:
@@ -66,3 +72,18 @@ so that firmware can wakeup host using this device side pin.
marvell,wakeup-pin = <3>;
};
};
+
+&pci_rootport {
+ mvl_wifi: wifi@0,0 {
+ compatible = "pci1b4b,2b42";
+ reg = <0x83010000 0x0 0x00000000 0x0 0x00100000
+ 0x83010000 0x0 0x00100000 0x0 0x00100000>;
+ pinctrl-names = "default";
+ pinctrl-0 = <&wlan_host_wake_l>;
+ wake-up {
+ interrupt-parent = <&gpio0>;
+ interrupts = <8 IRQ_TYPE_LEVEL_LOW>;
+ wakeup-source;
+ };
+ };
+};
--
2.20.1
^ permalink raw reply related
* Re: [RFC v1 12/19] RDMA/irdma: Implement device supported verb APIs
From: Gal Pressman @ 2019-02-24 14:35 UTC (permalink / raw)
To: Shiraz Saleem, dledford, jgg, davem
Cc: linux-rdma, netdev, mustafa.ismail, jeffrey.t.kirsher,
Yossi Leybovich
In-Reply-To: <20190215171107.6464-13-shiraz.saleem@intel.com>
On 15-Feb-19 19:10, Shiraz Saleem wrote:
> /**
> * irdma_dealloc_ucontext - deallocate the user context data structure
> * @context: user context created during alloc
> */
> static int irdma_dealloc_ucontext(struct ib_ucontext *context)
> {
> struct irdma_ucontext *ucontext = to_ucontext(context);
> unsigned long flags;
>
> spin_lock_irqsave(&ucontext->cq_reg_mem_list_lock, flags);
> if (!list_empty(&ucontext->cq_reg_mem_list)) {
> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
> return -EBUSY;
> }
> spin_unlock_irqrestore(&ucontext->cq_reg_mem_list_lock, flags);
>
> spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
> if (!list_empty(&ucontext->qp_reg_mem_list)) {
> spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
> return -EBUSY;
Drivers are not permitted to fail dealloc_ucontext.
> }
> spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
> kfree(ucontext);
>
> return 0;
> }
> +/**> + * irdma_disassociate_ucontext - Disassociate user context> + * @context: ib user context> + */> +static void irdma_disassociate_ucontext(struct ib_ucontext *context)
> +{
> +}
What's the motivation for a nop callback (over not implementing the
function)?
> +/**
> + * irdma_alloc_pd - allocate protection domain
> + * @pd: PD pointer
> + * @context: user context created during alloc
> + * @udata: user data
> + */
> +static int irdma_alloc_pd(struct ib_pd *pd,
> + struct ib_ucontext *context,
> + struct ib_udata *udata)
> +{
> + struct irdma_pd *iwpd = to_iwpd(pd);
> + struct irdma_device *iwdev = to_iwdev(pd->device);
> + struct irdma_sc_dev *dev = &iwdev->rf->sc_dev;
> + struct irdma_pci_f *rf = iwdev->rf;
> + struct irdma_alloc_pd_resp uresp = {};
> + struct irdma_sc_pd *sc_pd;
> + struct irdma_ucontext *ucontext;
> + u32 pd_id = 0;
> + int err;
> +
> + if (iwdev->closing)
> + return -ENODEV;
> +
> + err = irdma_alloc_rsrc(rf, rf->allocated_pds, rf->max_pd, &pd_id,
> + &rf->next_pd);
> + if (err)
> + return err;
> +
> + sc_pd = &iwpd->sc_pd;
> + if (context) {
I think this should be 'if (udata)', this applies to many other places in this driver.
> + ucontext = to_ucontext(context);
> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, ucontext->abi_ver);
> + uresp.pd_id = pd_id;
> + if (ib_copy_to_udata(udata, &uresp, sizeof(uresp))) {
> + err = -EFAULT;
> + goto error;
> + }
> + } else {
> + dev->iw_pd_ops->pd_init(dev, sc_pd, pd_id, -1);
> + }
> +
> + irdma_add_pdusecount(iwpd);
> +
> + return 0;
> +error:
> + irdma_free_rsrc(rf, rf->allocated_pds, pd_id);
> +
> + return err;
> +}
> +/**
> + * irdma_create_qp - create qp
> + * @ibpd: ptr of pd
> + * @init_attr: attributes for qp
> + * @udata: user data for create qp
> + */
> +static struct ib_qp *irdma_create_qp(struct ib_pd *ibpd,
> + struct ib_qp_init_attr *init_attr,
> + struct ib_udata *udata)
> +{
> + struct irdma_pd *iwpd = to_iwpd(ibpd);
> + struct irdma_device *iwdev = to_iwdev(ibpd->device);
> + struct irdma_pci_f *rf = iwdev->rf;
> + struct irdma_cqp *iwcqp = &rf->cqp;
> + struct irdma_qp *iwqp;
> + struct irdma_ucontext *ucontext;
> + struct irdma_create_qp_req req;
> + struct irdma_create_qp_resp uresp = {};
> + struct i40iw_create_qp_resp uresp_gen1 = {};
> + u32 qp_num = 0;
> + void *mem;
> + enum irdma_status_code ret;
> + int err_code = 0;
> + int sq_size;
> + int rq_size;
> + struct irdma_sc_qp *qp;
> + struct irdma_sc_dev *dev = &rf->sc_dev;
> + struct irdma_qp_init_info init_info = {};
> + struct irdma_create_qp_info *qp_info;
> + struct irdma_cqp_request *cqp_request;
> + struct cqp_cmds_info *cqp_info;
> + struct irdma_qp_host_ctx_info *ctx_info;
> + struct irdma_iwarp_offload_info *iwarp_info;
> + struct irdma_roce_offload_info *roce_info;
> + struct irdma_udp_offload_info *udp_info;
> + unsigned long flags;
> +
> + if (iwdev->closing)
> + return ERR_PTR(-ENODEV);
> +
> + if (init_attr->create_flags)
> + return ERR_PTR(-EINVAL);
> +
> + if (init_attr->cap.max_inline_data > dev->hw_attrs.max_hw_inline)
> + init_attr->cap.max_inline_data = dev->hw_attrs.max_hw_inline;
> +
> + if (init_attr->cap.max_send_sge > dev->hw_attrs.max_hw_wq_frags)
> + init_attr->cap.max_send_sge = dev->hw_attrs.max_hw_wq_frags;
> +
> + if (init_attr->cap.max_recv_sge > dev->hw_attrs.max_hw_wq_frags)
> + init_attr->cap.max_recv_sge = dev->hw_attrs.max_hw_wq_frags;
AFAIK, you can change the requested values to be greater than or equal to
the values requested. I don't think you can change them to something smaller.
> +
> + sq_size = init_attr->cap.max_send_wr;
> + rq_size = init_attr->cap.max_recv_wr;
> +
> + init_info.vsi = &iwdev->vsi;
> + init_info.qp_uk_init_info.hw_attrs = &dev->hw_attrs;
> + init_info.qp_uk_init_info.sq_size = sq_size;
> + init_info.qp_uk_init_info.rq_size = rq_size;
> + init_info.qp_uk_init_info.max_sq_frag_cnt = init_attr->cap.max_send_sge;
> + init_info.qp_uk_init_info.max_rq_frag_cnt = init_attr->cap.max_recv_sge;
> + init_info.qp_uk_init_info.max_inline_data = init_attr->cap.max_inline_data;
> +
> + mem = kzalloc(sizeof(*iwqp), GFP_KERNEL);
> + if (!mem)
> + return ERR_PTR(-ENOMEM);
> +
> + iwqp = (struct irdma_qp *)mem;
> + iwqp->allocated_buf = mem;
'allocated_buf' feels redundant. Why is iwqp not sufficient?
> + qp = &iwqp->sc_qp;
> + qp->back_qp = (void *)iwqp;
> + qp->push_idx = IRDMA_INVALID_PUSH_PAGE_INDEX;
> +
> + if (irdma_allocate_dma_mem(dev->hw,
> + &iwqp->q2_ctx_mem,
> + IRDMA_Q2_BUF_SIZE + IRDMA_QP_CTX_SIZE,
> + 256)) {
> + err_code = -ENOMEM;
> + goto error;
> + }
> +
> + init_info.q2 = iwqp->q2_ctx_mem.va;
> + init_info.q2_pa = iwqp->q2_ctx_mem.pa;
> + init_info.host_ctx = (void *)init_info.q2 + IRDMA_Q2_BUF_SIZE;
> + init_info.host_ctx_pa = init_info.q2_pa + IRDMA_Q2_BUF_SIZE;
> +
> + if (init_attr->qp_type == IB_QPT_GSI && rf->sc_dev.is_pf)
> + qp_num = 1;
> + else
> + err_code = irdma_alloc_rsrc(rf, rf->allocated_qps, rf->max_qp,
> + &qp_num, &rf->next_qp);
> + if (err_code)
> + goto error;
> +
> + iwqp->iwdev = iwdev;
> + iwqp->iwpd = iwpd;
> + if (init_attr->qp_type == IB_QPT_GSI && !rf->sc_dev.is_pf)
> + iwqp->ibqp.qp_num = 1;
> + else
> + iwqp->ibqp.qp_num = qp_num;
> +
> + qp = &iwqp->sc_qp;
> + iwqp->iwscq = to_iwcq(init_attr->send_cq);
> + iwqp->iwrcq = to_iwcq(init_attr->recv_cq);
> + iwqp->host_ctx.va = init_info.host_ctx;
> + iwqp->host_ctx.pa = init_info.host_ctx_pa;
> + iwqp->host_ctx.size = IRDMA_QP_CTX_SIZE;
> +
> + init_info.pd = &iwpd->sc_pd;
> + init_info.qp_uk_init_info.qp_id = iwqp->ibqp.qp_num;
> + if (!rdma_protocol_roce(&iwdev->iwibdev->ibdev, 1))
> + init_info.qp_uk_init_info.first_sq_wq = 1;
> + iwqp->ctx_info.qp_compl_ctx = (uintptr_t)qp;
> + init_waitqueue_head(&iwqp->waitq);
> + init_waitqueue_head(&iwqp->mod_qp_waitq);
> +
> + if (rdma_protocol_roce(&iwdev->iwibdev->ibdev, 1)) {
> + if (init_attr->qp_type != IB_QPT_RC &&
> + init_attr->qp_type != IB_QPT_UD &&
> + init_attr->qp_type != IB_QPT_GSI) {
> + err_code = -EINVAL;
> + goto error;
> + }
> + } else {
> + if (init_attr->qp_type != IB_QPT_RC) {
> + err_code = -EINVAL;
> + goto error;
> + }
> + }
> +
> + if (iwdev->push_mode)
> + irdma_alloc_push_page(rf, qp);
> +
> + if (udata) {
> + err_code = ib_copy_from_udata(&req, udata, sizeof(req));
Perhaps ib_copy_from_udata(&req, udata, min(sizeof(req), udata->inlen)?
Applies to other call sites of ib_copy_from/to_udata as well.
> + if (err_code) {
> + irdma_debug(dev, IRDMA_DEBUG_ERR,
> + "ib_copy_from_data fail\n");
> + goto error;
> + }
> +
> + iwqp->ctx_info.qp_compl_ctx = req.user_compl_ctx;
> + iwqp->user_mode = 1;
> + ucontext = to_ucontext(ibpd->uobject->context);
> + if (req.user_wqe_bufs) {
> + spin_lock_irqsave(&ucontext->qp_reg_mem_list_lock, flags);
> + iwqp->iwpbl = irdma_get_pbl((unsigned long)req.user_wqe_bufs,
> + &ucontext->qp_reg_mem_list);
> + spin_unlock_irqrestore(&ucontext->qp_reg_mem_list_lock, flags);
> +
> + if (!iwqp->iwpbl) {
> + err_code = -ENODATA;
> + irdma_debug(dev, IRDMA_DEBUG_ERR,
> + "no pbl info\n");
> + goto error;
> + }
> + }
> + err_code = irdma_setup_virt_qp(iwdev, iwqp, &init_info);
> + } else {
> + err_code = irdma_setup_kmode_qp(iwdev, iwqp, &init_info);
> + }
> +
> + if (err_code) {
> + irdma_debug(dev, IRDMA_DEBUG_ERR, "setup qp failed\n");
> + goto error;
> + }
> +
> + if (rdma_protocol_roce(&iwdev->iwibdev->ibdev, 1)) {
> + if (init_attr->qp_type == IB_QPT_RC) {
> + init_info.type = IRDMA_QP_TYPE_ROCE_RC;
> + init_info.qp_uk_init_info.qp_caps =
> + IRDMA_SEND_WITH_IMM | IRDMA_WRITE_WITH_IMM | IRDMA_ROCE;
> + } else {
> + init_info.type = IRDMA_QP_TYPE_ROCE_UD;
> + init_info.qp_uk_init_info.qp_caps = IRDMA_SEND_WITH_IMM | IRDMA_ROCE;
> + }
> + } else {
> + init_info.type = IRDMA_QP_TYPE_IWARP;
> + init_info.qp_uk_init_info.qp_caps = IRDMA_WRITE_WITH_IMM;
> + }
> +
> + ret = dev->iw_priv_qp_ops->qp_init(qp, &init_info);
> + if (ret) {
> + err_code = -EPROTO;
> + irdma_debug(dev, IRDMA_DEBUG_ERR, "qp_init fail\n");
> + goto error;
> + }
> +
> + ctx_info = &iwqp->ctx_info;
> + if (rdma_protocol_roce(&iwdev->iwibdev->ibdev, 1)) {
> + iwqp->ctx_info.roce_info = &iwqp->roce_info;
> + iwqp->ctx_info.udp_info = &iwqp->udp_info;
> + udp_info = &iwqp->udp_info;
> + udp_info->snd_mss = irdma_roce_mtu(iwdev->vsi.mtu);
> + udp_info->cwnd = 0x400;
> + udp_info->src_port = 0xc000;
> + udp_info->dst_port = ROCE_V2_UDP_DPORT;
> + roce_info = &iwqp->roce_info;
> + ether_addr_copy(roce_info->mac_addr, iwdev->netdev->dev_addr);
> +
> + if (init_attr->qp_type == IB_QPT_GSI && !rf->sc_dev.is_pf)
> + roce_info->is_qp1 = true;
> + roce_info->rd_en = true;
> + roce_info->wr_rdresp_en = true;
> + roce_info->dctcp_en = iwdev->dctcp_en;
> + roce_info->ecn_en = iwdev->ecn_en;
> + roce_info->dcqcn_en = iwdev->roce_dcqcn_en;
> + roce_info->timely_en = iwdev->roce_timely_en;
> +
> + roce_info->ack_credits = 0x1E;
> + roce_info->ird_size = IRDMA_MAX_ENCODED_IRD_SIZE;
> + roce_info->ord_size = dev->hw_attrs.max_hw_ord;
> +
> + if (!iwqp->user_mode) {
> + roce_info->priv_mode_en = true;
> + roce_info->fast_reg_en = true;
> + roce_info->udprivcq_en = true;
> + }
> + roce_info->roce_tver = 0;
> + } else {
> + iwqp->ctx_info.iwarp_info = &iwqp->iwarp_info;
> + iwarp_info = &iwqp->iwarp_info;
> + ether_addr_copy(iwarp_info->mac_addr, iwdev->netdev->dev_addr);
> + iwarp_info->rd_en = true;
> + iwarp_info->wr_rdresp_en = true;
> + iwarp_info->ib_rd_en = true;
> + if (!iwqp->user_mode) {
> + iwarp_info->priv_mode_en = true;
> + iwarp_info->fast_reg_en = true;
> + }
> + iwarp_info->ddp_ver = 1;
> + iwarp_info->rdmap_ver = 1;
> + ctx_info->iwarp_info_valid = true;
> + }
> +
> + ctx_info->send_cq_num = iwqp->iwscq->sc_cq.cq_uk.cq_id;
> + ctx_info->rcv_cq_num = iwqp->iwrcq->sc_cq.cq_uk.cq_id;
> + if (qp->push_idx == IRDMA_INVALID_PUSH_PAGE_INDEX) {
> + ctx_info->push_mode_en = false;
> + } else {
> + ctx_info->push_mode_en = true;
> + ctx_info->push_idx = qp->push_idx;
> + }
> +
> + if (rdma_protocol_roce(&iwdev->iwibdev->ibdev, 1)) {
> + ret =
> + dev->iw_priv_qp_ops->qp_setctx_roce(&iwqp->sc_qp,
> + iwqp->host_ctx.va,
> + ctx_info);
> + } else {
> + ret = dev->iw_priv_qp_ops->qp_setctx(&iwqp->sc_qp,
> + iwqp->host_ctx.va,
> + ctx_info);
> + ctx_info->iwarp_info_valid = false;
> + }
> +
> + cqp_request = irdma_get_cqp_request(iwcqp, true);
> + if (!cqp_request) {
> + err_code = -ENOMEM;
> + goto error;
> + }
> +
> + cqp_info = &cqp_request->info;
> + qp_info = &cqp_request->info.in.u.qp_create.info;
> + memset(qp_info, 0, sizeof(*qp_info));
> + qp_info->mac_valid = true;
> + qp_info->cq_num_valid = true;
> + qp_info->next_iwarp_state = IRDMA_QP_STATE_IDLE;
> +
> + cqp_info->cqp_cmd = IRDMA_OP_QP_CREATE;
> + cqp_info->post_sq = 1;
> + cqp_info->in.u.qp_create.qp = qp;
> + cqp_info->in.u.qp_create.scratch = (uintptr_t)cqp_request;
> + ret = irdma_handle_cqp_op(rf, cqp_request);
> + if (ret) {
> + irdma_debug(dev, IRDMA_DEBUG_ERR, "CQP-OP QP create fail");
> + err_code = -ENOMEM;
> + goto error;
> + }
> +
> + irdma_add_ref(&iwqp->ibqp);
> + spin_lock_init(&iwqp->lock);
> + spin_lock_init(&iwqp->sc_qp.pfpdu.lock);
> + iwqp->sig_all = (init_attr->sq_sig_type == IB_SIGNAL_ALL_WR) ? 1 : 0;
> + rf->qp_table[qp_num] = iwqp;
> + irdma_add_pdusecount(iwqp->iwpd);
> + irdma_add_devusecount(iwdev);
> + if (udata) {
> + if (iwdev->rf->sc_dev.hw_attrs.hw_rev == IRDMA_GEN_1) {
> + uresp_gen1.lsmm = 1;
> + uresp_gen1.actual_sq_size = sq_size;
> + uresp_gen1.actual_rq_size = rq_size;
> + uresp_gen1.qp_id = qp_num;
> + uresp_gen1.push_idx = qp->push_idx;
> + uresp_gen1.lsmm = 1;
> + err_code = ib_copy_to_udata(udata, &uresp_gen1, sizeof(uresp_gen1));
> + } else {
> + if (rdma_protocol_iwarp(&iwdev->iwibdev->ibdev, 1))
> + uresp.lsmm = 1;
> + uresp.actual_sq_size = sq_size;
> + uresp.actual_rq_size = rq_size;
> + uresp.qp_id = qp_num;
> + uresp.push_idx = qp->push_idx;
> + uresp.qp_caps = qp->qp_uk.qp_caps;
> +
> + err_code = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> + }
> + if (err_code) {
> + irdma_debug(dev, IRDMA_DEBUG_ERR, "copy_to_udata failed\n");
> + irdma_destroy_qp(&iwqp->ibqp);
> + return ERR_PTR(err_code);
> + }
> + }
> + init_completion(&iwqp->sq_drained);
> + init_completion(&iwqp->rq_drained);
> + return &iwqp->ibqp;
> +
> +error:
> + irdma_free_qp_rsrc(iwdev, iwqp, qp_num);
> +
> + return ERR_PTR(err_code);
> +}
> +
> +/**
> + * irdma_query - query qp attributes
> + * @ibqp: qp pointer
> + * @attr: attributes pointer
> + * @attr_mask: Not used
> + * @init_attr: qp attributes to return
> + */
> +static int irdma_query_qp(struct ib_qp *ibqp,
> + struct ib_qp_attr *attr,
> + int attr_mask,
> + struct ib_qp_init_attr *init_attr)
> +{
> + struct irdma_qp *iwqp = to_iwqp(ibqp);
> + struct irdma_sc_qp *qp = &iwqp->sc_qp;
> +
> + attr->qp_state = iwqp->ibqp_state;
> + attr->cur_qp_state = iwqp->ibqp_state;
> + attr->qp_access_flags = 0;
> + attr->cap.max_send_wr = qp->qp_uk.sq_size - 1;
> + attr->cap.max_recv_wr = qp->qp_uk.rq_size - 1;
Why -1?
> + attr->cap.max_inline_data = qp->qp_uk.max_inline_data;
> + attr->cap.max_send_sge = qp->qp_uk.max_sq_frag_cnt;
> + attr->cap.max_recv_sge = qp->qp_uk.max_rq_frag_cnt;
> + attr->qkey = iwqp->roce_info.qkey;
> +
> + init_attr->event_handler = iwqp->ibqp.event_handler;
> + init_attr->qp_context = iwqp->ibqp.qp_context;
> + init_attr->send_cq = iwqp->ibqp.send_cq;
> + init_attr->recv_cq = iwqp->ibqp.recv_cq;
> + init_attr->srq = iwqp->ibqp.srq;
> + init_attr->cap = attr->cap;
> +
> + return 0;
> +}
> +
> +/**
> + * irdma_destroy_cq - destroy cq
> + * @ib_cq: cq pointer
> + */
> +static int irdma_destroy_cq(struct ib_cq *ib_cq)
> +{
> + struct irdma_cq *iwcq;
> + struct irdma_device *iwdev;
> + struct irdma_sc_cq *cq;
> +
> + if (!ib_cq) {
> + irdma_pr_err("ib_cq == NULL\n");
> + return 0;
> + }
Is this really needed? Which caller can pass NULL pointer?
> +
> + iwcq = to_iwcq(ib_cq);
> + iwdev = to_iwdev(ib_cq->device);
> + cq = &iwcq->sc_cq;
> + irdma_cq_wq_destroy(iwdev->rf, cq);
> + cq_free_rsrc(iwdev->rf, iwcq);
> + kfree(iwcq);
> + irdma_rem_devusecount(iwdev);
> +
> + return 0;
> +}
> +> +/**
> + * irdma_create_stag - create random stag
> + * @iwdev: iwarp device
> + */
> +static u32 irdma_create_stag(struct irdma_device *iwdev)
> +{
> + u32 stag = 0;
> + u32 stag_index = 0;
> + u32 next_stag_index;
> + u32 driver_key;
> + u32 random;
> + u8 consumer_key;
> + int ret;
> +
> + get_random_bytes(&random, sizeof(random));
> + consumer_key = (u8)random;
> +
> + driver_key = random & ~iwdev->rf->mr_stagmask;
> + next_stag_index = (random & iwdev->rf->mr_stagmask) >> 8;
> + next_stag_index %= iwdev->rf->max_mr;
> +
> + ret = irdma_alloc_rsrc(iwdev->rf,
> + iwdev->rf->allocated_mrs, iwdev->rf->max_mr,
> + &stag_index, &next_stag_index);
> + if (!ret) {
> + stag = stag_index << IRDMA_CQPSQ_STAG_IDX_S;
> + stag |= driver_key;
> + stag += (u32)consumer_key;
> + irdma_add_devusecount(iwdev);
> + }
This is confusing IMHO, better to test for 'if (ret)' and keep the main flow
unindented.
> + return stag;
> +}
> +
> +/**
> + * board_id_show
> + */
> +static ssize_t board_id_show(struct device *dev,
> + struct device_attribute *attr,
> + char *buf)
> +{
> + return sprintf(buf, "%.*s\n", 32, "IRDMA Board ID");
That doesn't add much information.
> +}
> +
> +static DEVICE_ATTR_RO(hw_rev);
> +static DEVICE_ATTR_RO(hca_type);
> +static DEVICE_ATTR_RO(board_id);
> +
> +static struct attribute *irdma_dev_attributes[] = {
> + &dev_attr_hw_rev.attr,
> + &dev_attr_hca_type.attr,
> + &dev_attr_board_id.attr,
> + NULL
> +};
> +
> +static const struct attribute_group irdma_attr_group = {
> + .attrs = irdma_dev_attributes,
> +};
> +
> +/**
> + * irdma_modify_port Modify port properties
> + * @ibdev: device pointer from stack
> + * @port: port number
> + * @port_modify_mask: mask for port modifications
> + * @props: port properties
> + */
> +static int irdma_modify_port(struct ib_device *ibdev,
> + u8 port,
> + int port_modify_mask,
> + struct ib_port_modify *props)
> +{
> + return 0;
> +}
Same question as disacossiate_ucontext.
> +
> +/**
> + * irdma_query_gid_roce - Query port GID for Roce
> + * @ibdev: device pointer from stack
> + * @port: port number
> + * @index: Entry index
> + * @gid: Global ID
> + */
> +static int irdma_query_gid_roce(struct ib_device *ibdev,
> + u8 port,
> + int index,
> + union ib_gid *gid)
> +{
> + int ret;
> +
> + ret = rdma_query_gid(ibdev, port, index, gid);
> + if (ret == -EAGAIN) {
I can't see a path where rdma_query_gid returns -EAGAIN.
> + memcpy(gid, &zgid, sizeof(*gid));
> + return 0;
> + }
> +
> + return ret;
> +}
> +
> +/**
> + * irdma_create_ah - create address handle
> + * @ibpd: ptr to protection domain
> + * @ah_attr: address handle attributes
'ah_attr' -> 'attr', missing flags and udata.
> + *
> + * returns a pointer to an address handle
> + */
> +static struct ib_ah *irdma_create_ah(struct ib_pd *ibpd,
> + struct rdma_ah_attr *attr,
> + u32 flags,
> + struct ib_udata *udata)
> +{
> + struct irdma_pd *pd = to_iwpd(ibpd);
> + struct irdma_ah *ah;
> + const struct ib_gid_attr *sgid_attr;
> + struct irdma_device *iwdev = to_iwdev(ibpd->device);
> + struct irdma_pci_f *rf = iwdev->rf;
> + struct irdma_sc_ah *sc_ah;
> + u32 ah_id = 0;
> + struct irdma_ah_info *ah_info;
> + struct irdma_create_ah_resp uresp;
> + union {
> + struct sockaddr saddr;
> + struct sockaddr_in saddr_in;
> + struct sockaddr_in6 saddr_in6;
> + } sgid_addr, dgid_addr;
> + int err;
> + u8 dmac[ETH_ALEN];
> +
> + err = irdma_alloc_rsrc(rf, rf->allocated_ahs,
> + rf->max_ah, &ah_id, &rf->next_ah);
> + if (err)
> + return ERR_PTR(err);
> +
> + ah = kzalloc(sizeof(*ah), GFP_ATOMIC);
> + if (!ah) {
> + irdma_free_rsrc(rf, rf->allocated_ahs, ah_id);
> + return ERR_PTR(-ENOMEM);
> + }
> +
> + ah->pd = pd;
> + sc_ah = &ah->sc_ah;
> + sc_ah->ah_info.ah_idx = ah_id;
> + sc_ah->ah_info.vsi = &iwdev->vsi;
> + iwdev->rf->sc_dev.iw_uda_ops->init_ah(&rf->sc_dev, sc_ah);
> + ah->sgid_index = attr->grh.sgid_index;
> + sgid_attr = attr->grh.sgid_attr;
> + memcpy(&ah->dgid, &attr->grh.dgid, sizeof(ah->dgid));
> + rdma_gid2ip(&sgid_addr.saddr, &sgid_attr->gid);
> + rdma_gid2ip(&dgid_addr.saddr, &attr->grh.dgid);
> + ah->av.attrs = *attr;
> + ah->av.net_type = rdma_gid_attr_network_type(sgid_attr);
> + ah->av.sgid_addr.saddr = sgid_addr.saddr;
> + ah->av.dgid_addr.saddr = dgid_addr.saddr;
> + ah_info = &sc_ah->ah_info;
> + ah_info->ah = sc_ah;
> + ah_info->ah_idx = ah_id;
> + ah_info->pd_idx = pd->sc_pd.pd_id;
> + ether_addr_copy(ah_info->mac_addr, iwdev->netdev->dev_addr);
> + if (attr->ah_flags & IB_AH_GRH) {
> + ah_info->flow_label = attr->grh.flow_label;
> + ah_info->hop_ttl = attr->grh.hop_limit;
> + ah_info->tc_tos = attr->grh.traffic_class;
> + }
> +
> + ether_addr_copy(dmac, attr->roce.dmac);
> + if (rdma_gid_attr_network_type(sgid_attr) == RDMA_NETWORK_IPV4) {
> + ah_info->ipv4_valid = true;
> + ah_info->dest_ip_addr[0] =
> + ntohl(dgid_addr.saddr_in.sin_addr.s_addr);
> + ah_info->src_ip_addr[0] =
> + ntohl(sgid_addr.saddr_in.sin_addr.s_addr);
> + ah_info->do_lpbk = irdma_ipv4_is_lpb(ah_info->src_ip_addr[0],
> + ah_info->dest_ip_addr[0]);
> + if (ipv4_is_multicast(dgid_addr.saddr_in.sin_addr.s_addr))
> + irdma_mcast_mac(ah_info->dest_ip_addr, dmac, true);
> + } else {
> + irdma_copy_ip_ntohl(ah_info->dest_ip_addr,
> + dgid_addr.saddr_in6.sin6_addr.in6_u.u6_addr32);
> + irdma_copy_ip_ntohl(ah_info->src_ip_addr,
> + sgid_addr.saddr_in6.sin6_addr.in6_u.u6_addr32);
> + ah_info->do_lpbk = irdma_ipv6_is_lpb(ah_info->src_ip_addr,
> + ah_info->dest_ip_addr);
> + if (rdma_is_multicast_addr(&dgid_addr.saddr_in6.sin6_addr))
> + irdma_mcast_mac(ah_info->dest_ip_addr, dmac, false);
> + }
> + if (sgid_attr->ndev && is_vlan_dev(sgid_attr->ndev))
> + ah_info->vlan_tag = vlan_dev_vlan_id(sgid_attr->ndev);
> + else
> + ah_info->vlan_tag = IRDMA_NO_VLAN;
> +
> + ah_info->dst_arpindex = irdma_add_arp(iwdev->rf, ah_info->dest_ip_addr,
> + ah_info->ipv4_valid, dmac);
> +
> + if (ah_info->dst_arpindex == -1) {
> + err = -EINVAL;
> + goto error;
> + }
> +
> + if (ah_info->vlan_tag != 0xFFFF)
> + ah_info->insert_vlan_tag = true;
> +
> + err = irdma_ah_cqp_op(iwdev->rf, sc_ah, IRDMA_OP_AH_CREATE,
> + flags & RDMA_CREATE_AH_SLEEPABLE,
> + irdma_gsi_ud_qp_ah_cb, sc_ah);
> + if (err) {
> + irdma_debug(&rf->sc_dev, IRDMA_DEBUG_ERR,
> + "CQP-OP Create AH fail");
> + goto error;
> + }
> +
> + if (!(flags & RDMA_CREATE_AH_SLEEPABLE)) {
> + int cnt = CQP_COMPL_WAIT_TIME_MS * CQP_TIMEOUT_THRESHOLD;
> +
> + do {
> + irdma_cqp_ce_handler(rf, &rf->ccq.sc_cq);
> + mdelay(1);
> + } while (!sc_ah->ah_info.ah_valid && --cnt);
> +
> + if (!cnt) {
> + irdma_debug(&rf->sc_dev, IRDMA_DEBUG_ERR,
> + "CQP create AH timed out");
> + err = -ETIMEDOUT;
> + goto error;
> + }
> + }
> +
> + irdma_add_pdusecount(pd);
> + if (udata) {
> + uresp.ah_id = ah->sc_ah.ah_info.ah_idx;
> + err = ib_copy_to_udata(udata, &uresp, sizeof(uresp));
> + }
> + return &ah->ibah;
> +
> +error:
> + kfree(ah);
> + irdma_free_rsrc(iwdev->rf, iwdev->rf->allocated_ahs, ah_id);
> +
> + return ERR_PTR(err);
> +}
> +
> +/**
> + * irdma_destroy_ah - Destroy address handle
> + * @ah: pointer to address handle
Missing flags.
> + */
> +static int irdma_destroy_ah(struct ib_ah *ibah, u32 flags)
> +{
> + struct irdma_device *iwdev = to_iwdev(ibah->device);
> + struct irdma_ah *ah = to_iwah(ibah);
> + int err;
> +
> + if (!ah->sc_ah.ah_info.ah_valid)
> + return -EINVAL;
> +
> + err = irdma_ah_cqp_op(iwdev->rf, &ah->sc_ah, IRDMA_OP_AH_DESTROY,
> + flags & RDMA_DESTROY_AH_SLEEPABLE,
> + irdma_destroy_ah_cb, ah);
> + if (!err)
> + return 0;
Why are the rest of the cleanups only in case of error?
> +
> + irdma_free_rsrc(iwdev->rf, iwdev->rf->allocated_ahs,
> + ah->sc_ah.ah_info.ah_idx);
> + irdma_rem_pdusecount(ah->pd, iwdev);
> + kfree(ah);
> +
> + return 0;
> +}
> +
> +static __be64 irdma_mac_to_guid(struct net_device *ndev)
> +{
> + unsigned char *mac = ndev->dev_addr;
> + __be64 guid;
> + unsigned char *dst = (unsigned char *)&guid;
> +
> + dst[0] = mac[0] ^ 2;
> + dst[1] = mac[1];
> + dst[2] = mac[2];
> + dst[3] = 0xff;
> + dst[4] = 0xfe;
> + dst[5] = mac[3];
> + dst[6] = mac[4];
> + dst[7] = mac[5];
> +
> + return guid;
> +}
There's a variant of this function in irdma, bnxt_re, ocrdma and qedr.
Maybe it's time to provide it in common code?
^ permalink raw reply
* Re: [PATCH v2 2/2] drivers: net: phy: mdio-mux: Add support for Generic Mux controls
From: Andrew Lunn @ 2019-02-24 14:57 UTC (permalink / raw)
To: Pankaj Bansal
Cc: Leo Li, Peter Rosin, Florian Fainelli, Heiner Kallweit,
netdev@vger.kernel.org
In-Reply-To: <20190224141338.29907-2-pankaj.bansal@nxp.com>
> +static int mdio_mux_multiplexer_switch_fn(int current_child, int desired_child,
> + void *data)
> +{
> + struct platform_device *pdev;
> + struct mdio_mux_multiplexer_state *s;
> + int ret = 0;
> +
> + pdev = (struct platform_device *)data;
> + s = (struct mdio_mux_multiplexer_state *)platform_get_drvdata(pdev);
platform_get_drvdata() returns a void *. So you don't need the cast.
> + if (current_child ^ desired_child) {
> + if (current_child != -1)
> + ret = mux_control_deselect(s->muxc);
> + if (ret)
> + return ret;
Please use {} here, so you check the error when current_child != -1.
> +
> + ret = mux_control_select(s->muxc, desired_child);
> + if (!ret)
> + dev_dbg(&pdev->dev, "%s %d -> %d\n", __func__,
> + current_child, desired_child);
> + }
> +
> + return ret;
> +}
> +
> +static int mdio_mux_multiplexer_probe(struct platform_device *pdev)
> +{
> + struct device *dev = &pdev->dev;
> + struct mdio_mux_multiplexer_state *s;
> + int ret = 0;
> +
> + s = devm_kzalloc(&pdev->dev, sizeof(*s), GFP_KERNEL);
> + if (!s)
> + return -ENOMEM;
> +
> + s->muxc = devm_mux_control_get(dev, NULL);
> + if (IS_ERR(s->muxc)) {
> + ret = PTR_ERR(s->muxc);
> + if (ret != -EPROBE_DEFER)
> + dev_err(&pdev->dev, "Failed to get mux: %d\n", ret);
> + return ret;
> + }
> +
> + platform_set_drvdata(pdev, s);
> +
> + ret = mdio_mux_init(&pdev->dev, pdev->dev.of_node,
> + mdio_mux_multiplexer_switch_fn, &s->mux_handle,
> + pdev, NULL);
> +
> + return ret;
> +}
> +
> +static int mdio_mux_multiplexer_remove(struct platform_device *pdev)
> +{
> + struct mdio_mux_multiplexer_state *s = platform_get_drvdata(pdev);
> +
> + mdio_mux_uninit(s->mux_handle);
I've never used the multiplexer framework before. But i think you need
a call to mux_control_deselect() here in order to unlock the
multiplexer. Without that, it will deadlock when you reload the
module.
Andrew
^ permalink raw reply
* Re: [PATCH v2 1/2] dt-bindings: net: Add bindings for mdio mux consumers
From: Andrew Lunn @ 2019-02-24 15:00 UTC (permalink / raw)
To: Pankaj Bansal
Cc: Leo Li, Peter Rosin, Florian Fainelli, Heiner Kallweit,
Rob Herring, Mark Rutland, devicetree@vger.kernel.org,
netdev@vger.kernel.org
In-Reply-To: <20190224141338.29907-1-pankaj.bansal@nxp.com>
On Sun, Feb 24, 2019 at 08:49:21AM +0000, Pankaj Bansal wrote:
> When we use the bindings defined in Documentation/devicetree/bindings/mux
> to define mdio mux in producer and consumer terms, it results in two
> devices. one is mux producer and other is mux consumer.
>
> Add the bindings needed for Mdio mux consumer devices.
>
> Signed-off-by: Pankaj Bansal <pankaj.bansal@nxp.com>
Reviewed-by: Andrew Lunn <andrew@lunn.ch>
Andrew
^ permalink raw reply
* Re: stmmac / meson8b-dwmac
From: Simon Huelck @ 2019-02-24 15:00 UTC (permalink / raw)
To: Jerome Brunet, Jose Abreu, Martin Blumenstingl
Cc: linux-amlogic, Gpeppe.cavallaro, alexandre.torgue,
Emiliano Ingrassia, netdev
In-Reply-To: <d7de65c614ee788152300f6d3799fd537b438496.camel@baylibre.com>
Am 21.02.2019 um 18:46 schrieb Jerome Brunet:
> On Thu, 2019-02-21 at 18:27 +0100, Simon Huelck wrote:
>> Hi,
>>
>>
>>
>> this was changed recently, with a patch for the EEE stuff , see here:
>>
>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.0-rc7&id=e35e26b26e955c53e61c154ba26b9bb15da6b858
> Hu, I was not aware this finally went through. Good !
> As explained in the patch and by Jose, the GMAC should be using IRQ_LEVEL.
>
> The realtek PHY has EEE enabled by default. Having this enabled generates a
> lot of (Low Power) Interrupts.
>
> Previously, when the GMAC used IRQ_EDGE. Because it is wrong, we would
> eventually miss an IRQ and the interface would just die. Unfortunately, it was
> not that easy find out.
>
> 2 years ago, we just noticed that disabling EEE would make the failure go
> away. Forcing this EEE feature off through DT was merely a work around.
>
> Now that the real cause of the problem is known, there is no reason to keep
> this hack around.
>
> Whether EEE adds a performance penality and why, is another topic.
> As Jose pointed out, you can disable EEE at runtime, using ethtool.
>
> Jerome
>
Hi,
i tested the latest patches of **next-20190222, there were some stmmac
improvements.**
**
**
**For the topic i got , the performance stayed identical.**
**C:\Users\Simon\Downloads\iperf3.6_64bit\iperf3.6_64bit>iperf3.exe -c
10.10.11.1 -i1
warning: Ignoring nonsense TCP MSS 0
Connecting to host 10.10.11.1, port 5201
[ 5] local 10.10.11.100 port 50830 connected to 10.10.11.1 port 5201
[ ID] Interval Transfer Bitrate
[ 5] 0.00-1.00 sec 78.9 MBytes 661 Mbits/sec
[ 5] 1.00-2.00 sec 79.1 MBytes 664 Mbits/sec
[ 5] 2.00-3.00 sec 79.4 MBytes 666 Mbits/sec
[ 5] 3.00-4.00 sec 34.4 MBytes 288 Mbits/sec
[ 5] 4.00-5.00 sec 16.1 MBytes 135 Mbits/sec
[ 5] 5.00-6.00 sec 15.8 MBytes 132 Mbits/sec
[ 5] 6.00-7.00 sec 14.2 MBytes 120 Mbits/sec
[ 5] 7.00-8.00 sec 15.6 MBytes 131 Mbits/sec
[ 5] 8.00-9.00 sec 14.9 MBytes 125 Mbits/sec
[ 5] 9.00-10.00 sec 15.0 MBytes 126 Mbits/sec
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate
[ 5] 0.00-10.00 sec 363 MBytes 305 Mbits/sec sender
[ 5] 0.00-10.04 sec 363 MBytes 303 Mbits/sec
receiver
**
**its clearly visible when i activated the other stream for getting
duplex load ... The highest rate also stays alot under the possible
930MBits that i have seen earlier with 4.14.
**
**
**
**the parallel stream reached around 450Mbits , which almost sums up to
660Mbits. This is what i meant when i said that duplex might be broken.
**
**
**
**Connecting to host 10.10.11.100, port 5201
[ 5] local 10.10.11.1 port 38658 connected to 10.10.11.100 port 5201
[ ID] Interval Transfer Bitrate Retr Cwnd
[ 5] 0.00-1.00 sec 62.9 MBytes 528 Mbits/sec 0 65.6 KBytes
[ 5] 1.00-2.00 sec 56.9 MBytes 477 Mbits/sec 0 65.6 KBytes
[ 5] 2.00-3.00 sec 55.9 MBytes 469 Mbits/sec 0 65.6 KBytes
[ 5] 3.00-4.00 sec 53.0 MBytes 445 Mbits/sec 0 65.6 KBytes
[ 5] 4.00-5.00 sec 54.3 MBytes 455 Mbits/sec 0 65.6 KBytes
[ 5] 5.00-6.00 sec 54.8 MBytes 460 Mbits/sec 0 65.6 KBytes
[ 5] 6.00-7.00 sec 45.3 MBytes 380 Mbits/sec 0 65.6 KBytes
[ 5] 7.00-8.00 sec 51.2 MBytes 429 Mbits/sec 0 65.6 KBytes
[ 5] 8.00-9.00 sec 56.1 MBytes 470 Mbits/sec 0 65.6 KBytes
[ 5] 9.00-10.00 sec 55.3 MBytes 464 Mbits/sec 0 65.6 KBytes
- - - - - - - - - - - - - - - - - - - - - - - - -
[ ID] Interval Transfer Bitrate Retr
[ 5] 0.00-10.00 sec 546 MBytes 458 Mbits/sec 0 sender
[ 5] 0.00-10.00 sec 545 MBytes 457 Mbits/sec
receiver**
**
**
**regards,**
**Simon
**
**
**
^ permalink raw reply
* Re: [RFC v1 04/19] RDMA/irdma: Add driver framework definitions
From: Gal Pressman @ 2019-02-24 15:02 UTC (permalink / raw)
To: Shiraz Saleem, dledford, jgg, davem
Cc: linux-rdma, netdev, mustafa.ismail, jeffrey.t.kirsher
In-Reply-To: <20190215171107.6464-5-shiraz.saleem@intel.com>
On 15-Feb-19 19:10, Shiraz Saleem wrote:
> +/* client interface functions */
> +static const struct i40e_client_ops i40e_ops = {
> + .open = i40iw_open,
> + .close = i40iw_close,
> + .l2_param_change = i40iw_l2param_change,
> + .virtchnl_receive = NULL,
> + .vf_reset = NULL,
> + .vf_enable = NULL,
> + .vf_capable = NULL
NULL assignments are redundant.
> +};
> +
> diff --git a/drivers/infiniband/hw/irdma/irdma_if.c b/drivers/infiniband/hw/irdma/irdma_if.c
> new file mode 100644
> index 0000000..f7b89e9
> --- /dev/null
> +++ b/drivers/infiniband/hw/irdma/irdma_if.c
> @@ -0,0 +1,430 @@
> +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB
> +/* Copyright (c) 2019, Intel Corporation. */
> +
> +#include <linux/module.h>
> +#include <linux/moduleparam.h>
> +#include <ice_idc.h>
> +#include "main.h"
> +#include "ws.h"
> +#include "icrdma_hw.h"
> +
> +void irdma_add_dev_ref(struct irdma_sc_dev *dev)
> +{
> + try_module_get(THIS_MODULE);
> +}
> +
> +void irdma_put_dev_ref(struct irdma_sc_dev *dev)
> +{
> + module_put(THIS_MODULE);
> +}
What are these used for?
> +
> +/**
> + * irdma_find_iwdev - find a vsi device given a name
> + * @name: name of iwdev
> + */
Can't find uses of this function as well.
> +struct irdma_device *irdma_find_iwdev(const char *name)
> +{
> + struct irdma_handler *hdl;
> + struct list_head *pos;
> + struct list_head *tmp;
> + struct irdma_device *iwdev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&irdma_handler_lock, flags);
> + list_for_each_entry(hdl, &irdma_handlers, list) {
> + list_for_each_safe(pos, tmp, &hdl->rf.vsi_dev_list) {
> + iwdev = container_of(pos, struct irdma_device, list);
> + if (!strcmp(name, iwdev->iwibdev->ibdev.name)) {
> + spin_unlock_irqrestore(&irdma_handler_lock,
> + flags);
> + return iwdev;
> + }
> + }
> + }
> + spin_unlock_irqrestore(&irdma_handler_lock, flags);
> +
> + return NULL;
> +}
> +
^ permalink raw reply
* Re: stmmac / meson8b-dwmac
From: Simon Huelck @ 2019-02-24 15:02 UTC (permalink / raw)
To: Jerome Brunet, Jose Abreu, Martin Blumenstingl
Cc: linux-amlogic, Gpeppe.cavallaro, alexandre.torgue,
Emiliano Ingrassia, netdev
In-Reply-To: <8ec64936-c8fa-1f0e-68bf-2ad1d6e8f5d9@gmx.de>
Am 24.02.2019 um 16:00 schrieb Simon Huelck:
> Am 21.02.2019 um 18:46 schrieb Jerome Brunet:
>> On Thu, 2019-02-21 at 18:27 +0100, Simon Huelck wrote:
>>> Hi,
>>>
>>>
>>>
>>> this was changed recently, with a patch for the EEE stuff , see here:
>>>
>>> https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?h=v5.0-rc7&id=e35e26b26e955c53e61c154ba26b9bb15da6b858
>> Hu, I was not aware this finally went through. Good !
>> As explained in the patch and by Jose, the GMAC should be using IRQ_LEVEL.
>>
>> The realtek PHY has EEE enabled by default. Having this enabled generates a
>> lot of (Low Power) Interrupts.
>>
>> Previously, when the GMAC used IRQ_EDGE. Because it is wrong, we would
>> eventually miss an IRQ and the interface would just die. Unfortunately, it was
>> not that easy find out.
>>
>> 2 years ago, we just noticed that disabling EEE would make the failure go
>> away. Forcing this EEE feature off through DT was merely a work around.
>>
>> Now that the real cause of the problem is known, there is no reason to keep
>> this hack around.
>>
>> Whether EEE adds a performance penality and why, is another topic.
>> As Jose pointed out, you can disable EEE at runtime, using ethtool.
>>
>> Jerome
>>
> Hi,
>
>
> i tested the latest patches of **next-20190222, there were some stmmac
> improvements.**
>
> **
> **
>
> **For the topic i got , the performance stayed identical.**
>
> **C:\Users\Simon\Downloads\iperf3.6_64bit\iperf3.6_64bit>iperf3.exe -c
> 10.10.11.1 -i1
> warning: Ignoring nonsense TCP MSS 0
> Connecting to host 10.10.11.1, port 5201
> [ 5] local 10.10.11.100 port 50830 connected to 10.10.11.1 port 5201
> [ ID] Interval Transfer Bitrate
> [ 5] 0.00-1.00 sec 78.9 MBytes 661 Mbits/sec
> [ 5] 1.00-2.00 sec 79.1 MBytes 664 Mbits/sec
> [ 5] 2.00-3.00 sec 79.4 MBytes 666 Mbits/sec
> [ 5] 3.00-4.00 sec 34.4 MBytes 288 Mbits/sec
> [ 5] 4.00-5.00 sec 16.1 MBytes 135 Mbits/sec
> [ 5] 5.00-6.00 sec 15.8 MBytes 132 Mbits/sec
> [ 5] 6.00-7.00 sec 14.2 MBytes 120 Mbits/sec
> [ 5] 7.00-8.00 sec 15.6 MBytes 131 Mbits/sec
> [ 5] 8.00-9.00 sec 14.9 MBytes 125 Mbits/sec
> [ 5] 9.00-10.00 sec 15.0 MBytes 126 Mbits/sec
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate
> [ 5] 0.00-10.00 sec 363 MBytes 305 Mbits/sec sender
> [ 5] 0.00-10.04 sec 363 MBytes 303 Mbits/sec
> receiver
>
> **
>
> **its clearly visible when i activated the other stream for getting
> duplex load ... The highest rate also stays alot under the possible
> 930MBits that i have seen earlier with 4.14.
> **
>
> **
> **
>
> **the parallel stream reached around 450Mbits , which almost sums up to
> 660Mbits. This is what i meant when i said that duplex might be broken.
> **
>
> **
> **
>
> **Connecting to host 10.10.11.100, port 5201
> [ 5] local 10.10.11.1 port 38658 connected to 10.10.11.100 port 5201
> [ ID] Interval Transfer Bitrate Retr Cwnd
> [ 5] 0.00-1.00 sec 62.9 MBytes 528 Mbits/sec 0 65.6 KBytes
> [ 5] 1.00-2.00 sec 56.9 MBytes 477 Mbits/sec 0 65.6 KBytes
> [ 5] 2.00-3.00 sec 55.9 MBytes 469 Mbits/sec 0 65.6 KBytes
> [ 5] 3.00-4.00 sec 53.0 MBytes 445 Mbits/sec 0 65.6 KBytes
> [ 5] 4.00-5.00 sec 54.3 MBytes 455 Mbits/sec 0 65.6 KBytes
> [ 5] 5.00-6.00 sec 54.8 MBytes 460 Mbits/sec 0 65.6 KBytes
> [ 5] 6.00-7.00 sec 45.3 MBytes 380 Mbits/sec 0 65.6 KBytes
> [ 5] 7.00-8.00 sec 51.2 MBytes 429 Mbits/sec 0 65.6 KBytes
> [ 5] 8.00-9.00 sec 56.1 MBytes 470 Mbits/sec 0 65.6 KBytes
> [ 5] 9.00-10.00 sec 55.3 MBytes 464 Mbits/sec 0 65.6 KBytes
> - - - - - - - - - - - - - - - - - - - - - - - - -
> [ ID] Interval Transfer Bitrate Retr
> [ 5] 0.00-10.00 sec 546 MBytes 458 Mbits/sec 0 sender
> [ 5] 0.00-10.00 sec 545 MBytes 457 Mbits/sec
> receiver**
>
> **
> **
>
> **regards,**
>
> **Simon
> **
>
> **
> **
>
Hi,
and another thing that i recognized , TX from my odroid stays around 450
Mbits ( is not affected by RX). but RX suffers alot from the TX stuff (
the max sum of RX/TX seems to be the magical 650MBits)
regards,
Simon
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Andrew Lunn @ 2019-02-24 15:04 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Russell King - ARM Linux admin, Florian Fainelli,
netdev@vger.kernel.org
In-Reply-To: <6dd3f82a-eea0-c9b4-8dd6-f2f313578b1f@gmail.com>
> I think what's not correct is that phydev->autoneg is set
> (by phy_device_create) for a fixed link.
Fixed-link tries to emulate auto-neg:
bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
Maybe it needs better emulation of auto-neg?
Andrew
^ permalink raw reply
* [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
From: Tariq Toukan @ 2019-02-24 15:08 UTC (permalink / raw)
To: John W. Linville; +Cc: netdev, Eran Ben Elisha, Aya Levin, Tariq Toukan
From: Aya Levin <ayal@mellanox.com>
Introduce 50Gbps per lane link modes and 200Gbps speed, update print
functions and initialization functions accordingly.
In addition, update related man page accordingly.
Signed-off-by: Aya Levin <ayal@mellanox.com>
Signed-off-by: Tariq Toukan <tariqt@mellanox.com>
---
ethtool-copy.h | 19 ++++++++++++++++++-
ethtool.8.in | 15 +++++++++++++++
ethtool.c | 52 ++++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 85 insertions(+), 1 deletion(-)
diff --git a/ethtool-copy.h b/ethtool-copy.h
index 6bfbb85f9402..de459900b2d3 100644
--- a/ethtool-copy.h
+++ b/ethtool-copy.h
@@ -1455,15 +1455,31 @@ enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_FEC_NONE_BIT = 49,
ETHTOOL_LINK_MODE_FEC_RS_BIT = 50,
ETHTOOL_LINK_MODE_FEC_BASER_BIT = 51,
+ ETHTOOL_LINK_MODE_50000baseKR_Full_BIT = 52,
+ ETHTOOL_LINK_MODE_50000baseSR_Full_BIT = 53,
+ ETHTOOL_LINK_MODE_50000baseCR_Full_BIT = 54,
+ ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT = 55,
+ ETHTOOL_LINK_MODE_50000baseDR_Full_BIT = 56,
+ ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT = 57,
+ ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT = 58,
+ ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT = 59,
+ ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT = 60,
+ ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT = 61,
+ ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT = 62,
+ ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT = 63,
+ ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT = 64,
+ ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT = 65,
+ ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT = 66,
/* Last allowed bit for __ETHTOOL_LINK_MODE_LEGACY_MASK is bit
* 31. Please do NOT define any SUPPORTED_* or ADVERTISED_*
* macro for bits > 31. The only way to use indices > 31 is to
* use the new ETHTOOL_GLINKSETTINGS/ETHTOOL_SLINKSETTINGS API.
*/
+ ETHTOOL_LINK_MODE_LAST,
__ETHTOOL_LINK_MODE_LAST
- = ETHTOOL_LINK_MODE_FEC_BASER_BIT,
+ = ETHTOOL_LINK_MODE_LAST - 1,
};
#define __ETHTOOL_LINK_MODE_LEGACY_MASK(base_name) \
@@ -1571,6 +1587,7 @@ enum ethtool_link_mode_bit_indices {
#define SPEED_50000 50000
#define SPEED_56000 56000
#define SPEED_100000 100000
+#define SPEED_200000 200000
#define SPEED_UNKNOWN -1
diff --git a/ethtool.8.in b/ethtool.8.in
index 5a26cff5fb33..64ce0711ad5f 100644
--- a/ethtool.8.in
+++ b/ethtool.8.in
@@ -650,6 +650,11 @@ lB l lB.
0x400000000 50000baseCR2 Full
0x800000000 50000baseKR2 Full
0x10000000000 50000baseSR2 Full
+0x10000000000000 50000baseKR Full
+0x20000000000000 50000baseSR Full
+0x40000000000000 50000baseCR Full
+0x80000000000000 50000baseLR_ER_FR Full
+0x100000000000000 50000baseDR Full
0x8000000 56000baseKR4 Full
0x10000000 56000baseCR4 Full
0x20000000 56000baseSR4 Full
@@ -658,6 +663,16 @@ lB l lB.
0x2000000000 100000baseSR4 Full
0x4000000000 100000baseCR4 Full
0x8000000000 100000baseLR4_ER4 Full
+0x200000000000000 100000baseKR2 Full
+0x400000000000000 100000baseSR2 Full
+0x800000000000000 100000baseCR2 Full
+0x1000000000000000 100000baseLR2_ER2_FR2 Full
+0x2000000000000000 100000baseDR2 Full
+0x4000000000000000 200000baseKR4 Full
+0x8000000000000000 200000baseSR4 Full
+0x10000000000000000 200000baseLR4_ER4_FR4 Full
+0x20000000000000000 200000baseDR4 Full
+0x40000000000000000 200000baseCR4 Full
.TE
.TP
.BI phyad \ N
diff --git a/ethtool.c b/ethtool.c
index fb4c0886ca84..1a23be818ec1 100644
--- a/ethtool.c
+++ b/ethtool.c
@@ -530,6 +530,21 @@ static void init_global_link_mode_masks(void)
ETHTOOL_LINK_MODE_10000baseER_Full_BIT,
ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+ ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+ ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT,
+ ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
};
static const enum ethtool_link_mode_bit_indices
additional_advertised_flags_bits[] = {
@@ -689,6 +704,36 @@ static void dump_link_caps(const char *prefix, const char *an_prefix,
"2500baseT/Full" },
{ 0, ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
"5000baseT/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseKR_Full_BIT,
+ "50000baseKR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseSR_Full_BIT,
+ "50000baseSR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseCR_Full_BIT,
+ "50000baseCR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseLR_ER_FR_Full_BIT,
+ "50000baseLR_ER_FR/Full" },
+ { 0, ETHTOOL_LINK_MODE_50000baseDR_Full_BIT,
+ "50000baseDR/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseKR2_Full_BIT,
+ "100000baseKR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseSR2_Full_BIT,
+ "100000baseSR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseCR2_Full_BIT,
+ "100000baseCR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseLR2_ER2_FR2_Full_BIT,
+ "100000baseLR2_ER2_FR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_100000baseDR2_Full_BIT,
+ "100000baseDR2/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT,
+ "200000baseKR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT,
+ "200000baseSR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT,
+ "200000baseLR4_ER4_FR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT,
+ "200000baseDR4/Full" },
+ { 0, ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT,
+ "200000baseCR4/Full" },
};
int indent;
int did1, new_line_pend, i;
@@ -2958,6 +3003,13 @@ static int do_sset(struct cmd_context *ctx)
else if (speed_wanted == SPEED_10000 &&
duplex_wanted == DUPLEX_FULL)
adv_bit = ETHTOOL_LINK_MODE_10000baseT_Full_BIT;
+ else if (speed_wanted == SPEED_20000 &&
+ duplex_wanted == DUPLEX_FULL)
+ adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT &
+ ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT &
+ ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT &
+ ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT &
+ ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT;
if (adv_bit >= 0) {
advertising_wanted = mask_advertising_wanted;
--
1.8.3.1
^ permalink raw reply related
* Re: No traffic with Marvell switch and latest linux-next
From: Russell King - ARM Linux admin @ 2019-02-24 15:15 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190224150403.GF26626@lunn.ch>
On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
> > I think what's not correct is that phydev->autoneg is set
> > (by phy_device_create) for a fixed link.
>
> Fixed-link tries to emulate auto-neg:
>
> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
>
> Maybe it needs better emulation of auto-neg?
Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
(BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
so according to 802.3-2015, we should not be setting 1.5
(BMSR_ANEGCOMPLETE).
However, swphy does try to emulate autonegotiation - we do have cases
where swphy is used in situations where the speed and duplex are not
fixed. It returns an emulated link partner advertisement for the
current speed, which would suggest that we should set BMCR_ANENABLE.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH iproute2-next v2 00/19] Export object IDs to users
From: David Ahern @ 2019-02-24 15:16 UTC (permalink / raw)
To: Leon Romanovsky
Cc: Leon Romanovsky, netdev, RDMA mailing list, Stephen Hemminger,
Steve Wise
In-Reply-To: <20190223091528.8509-1-leon@kernel.org>
On 2/23/19 4:15 AM, Leon Romanovsky wrote:
> From: Leon Romanovsky <leonro@mellanox.com>
>
> Changelog:
> v1->v2:
> * Fixed commit messages
> * Added Steve's ROB
> v0->v1:
> * Rebased to latest iproute2-next
> * Added latest rdma_netlink.h and updated commit message to point
> to kernel SHA commit.
>
> This series adds ability to present and query all known to rdmatool
> object by their respective, unique IDs (e.g. pdn. mrn, cqn e.t.c).
> All objects which have "parent" object has this information too.
>
applied to iproute2-next. Thanks, Leon.
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Heiner Kallweit @ 2019-02-24 15:28 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Andrew Lunn
Cc: Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190224151555.xsopwkuicsop65rw@shell.armlinux.org.uk>
On 24.02.2019 16:15, Russell King - ARM Linux admin wrote:
> On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
>>> I think what's not correct is that phydev->autoneg is set
>>> (by phy_device_create) for a fixed link.
>>
>> Fixed-link tries to emulate auto-neg:
>>
>> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
>>
>> Maybe it needs better emulation of auto-neg?
>
> Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
> (BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
> so according to 802.3-2015, we should not be setting 1.5
> (BMSR_ANEGCOMPLETE).
>
> However, swphy does try to emulate autonegotiation - we do have cases
> where swphy is used in situations where the speed and duplex are not
> fixed. It returns an emulated link partner advertisement for the
> current speed, which would suggest that we should set BMCR_ANENABLE.
>
If we emulate auto-neg, then it's not needed to set the speed bits
in BMCR. Also what just comes to my mind, certain speeds like 1000BaseT
don't support forced mode. So we may have to go with auto-neg.
To avoid the original issue it should be sufficient to copy
supported -> advertising at a suited place.
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Russell King - ARM Linux admin @ 2019-02-24 15:31 UTC (permalink / raw)
To: Andrew Lunn; +Cc: Heiner Kallweit, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190223234235.GA26626@lunn.ch>
On Sun, Feb 24, 2019 at 12:42:35AM +0100, Andrew Lunn wrote:
> Looking forward, at some point we are going to have to make fixed-link
> support higher speeds. That probably means we need a swphy-c45 which
> emulates the standard registers for 2.5G, 5G and 10G. At that point
> genphy will not work...
Do we _need_ to emulate Clause 45 PHYs? Today, the MII interface does
not work with Clause 45 PHYs, so there is no userspace accessing these
PHYs out there. I have a patch that adds support for the MII ioctls
which gives me the ability to poke around in the 88x3310 for
investigatory / debug purposes.
However, I don't see the point of adding what would be very complex
Clause 45 support (we'd have to emulate the PMA/PMD, PCS and AN as a
minimum) when we have the ethtool API, and then we have the issue
that not all advertisement modes are standardised in Clause 45 -
there are some missing.
As I understand it, the swphy for fixed-links only exists because we
have existing tooling that expects Clause 22 PHYs to exist. This will
break even if we were to add Clause 45 support as many bits in the
first 16 registers have different meanings.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Russell King - ARM Linux admin @ 2019-02-24 15:34 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <403607a0-16d8-15b6-f0aa-b8b13793d401@gmail.com>
On Sun, Feb 24, 2019 at 04:28:32PM +0100, Heiner Kallweit wrote:
> On 24.02.2019 16:15, Russell King - ARM Linux admin wrote:
> > On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
> >>> I think what's not correct is that phydev->autoneg is set
> >>> (by phy_device_create) for a fixed link.
> >>
> >> Fixed-link tries to emulate auto-neg:
> >>
> >> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
> >>
> >> Maybe it needs better emulation of auto-neg?
> >
> > Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
> > (BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
> > so according to 802.3-2015, we should not be setting 1.5
> > (BMSR_ANEGCOMPLETE).
> >
> > However, swphy does try to emulate autonegotiation - we do have cases
> > where swphy is used in situations where the speed and duplex are not
> > fixed. It returns an emulated link partner advertisement for the
> > current speed, which would suggest that we should set BMCR_ANENABLE.
> >
> If we emulate auto-neg, then it's not needed to set the speed bits
> in BMCR. Also what just comes to my mind, certain speeds like 1000BaseT
> don't support forced mode. So we may have to go with auto-neg.
Sure.
> To avoid the original issue it should be sufficient to copy
> supported -> advertising at a suited place.
Why bother - the software PHY emulation is an emulation to allow
existing userspace that pre-dates the ethtool API to get some link
parameters. If we augment the PHY emulation in non-standard ways,
userspace will need to be updated to handle those non-standard
ways. If userspace needs to be updated, why not just bite the
bullet and update to ethtool APIs rather than adding more
complication through an emulation layer?
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Heiner Kallweit @ 2019-02-24 15:39 UTC (permalink / raw)
To: Russell King - ARM Linux admin
Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <20190224153449.gg3c3fyryztdj3vm@shell.armlinux.org.uk>
On 24.02.2019 16:34, Russell King - ARM Linux admin wrote:
> On Sun, Feb 24, 2019 at 04:28:32PM +0100, Heiner Kallweit wrote:
>> On 24.02.2019 16:15, Russell King - ARM Linux admin wrote:
>>> On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
>>>>> I think what's not correct is that phydev->autoneg is set
>>>>> (by phy_device_create) for a fixed link.
>>>>
>>>> Fixed-link tries to emulate auto-neg:
>>>>
>>>> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
>>>>
>>>> Maybe it needs better emulation of auto-neg?
>>>
>>> Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
>>> (BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
>>> so according to 802.3-2015, we should not be setting 1.5
>>> (BMSR_ANEGCOMPLETE).
>>>
>>> However, swphy does try to emulate autonegotiation - we do have cases
>>> where swphy is used in situations where the speed and duplex are not
>>> fixed. It returns an emulated link partner advertisement for the
>>> current speed, which would suggest that we should set BMCR_ANENABLE.
>>>
>> If we emulate auto-neg, then it's not needed to set the speed bits
>> in BMCR. Also what just comes to my mind, certain speeds like 1000BaseT
>> don't support forced mode. So we may have to go with auto-neg.
>
> Sure.
>
>> To avoid the original issue it should be sufficient to copy
>> supported -> advertising at a suited place.
>
Sorry, seems this wasn't clear enough. I don't mean to change
swphy but the user side.
> Why bother - the software PHY emulation is an emulation to allow
> existing userspace that pre-dates the ethtool API to get some link
> parameters. If we augment the PHY emulation in non-standard ways,
> userspace will need to be updated to handle those non-standard
> ways. If userspace needs to be updated, why not just bite the
> bullet and update to ethtool APIs rather than adding more
> complication through an emulation layer?
>
It's not only userspace. Based on my limited knowledge of DSA this
code also uses e.g. genphy_read_status() with a fixed link.
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Russell King - ARM Linux admin @ 2019-02-24 15:49 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Andrew Lunn, Florian Fainelli, netdev@vger.kernel.org
In-Reply-To: <bdca070b-53ea-72ab-4864-e5d36048553f@gmail.com>
On Sun, Feb 24, 2019 at 04:39:30PM +0100, Heiner Kallweit wrote:
> On 24.02.2019 16:34, Russell King - ARM Linux admin wrote:
> > On Sun, Feb 24, 2019 at 04:28:32PM +0100, Heiner Kallweit wrote:
> >> On 24.02.2019 16:15, Russell King - ARM Linux admin wrote:
> >>> On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
> >>>>> I think what's not correct is that phydev->autoneg is set
> >>>>> (by phy_device_create) for a fixed link.
> >>>>
> >>>> Fixed-link tries to emulate auto-neg:
> >>>>
> >>>> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
> >>>>
> >>>> Maybe it needs better emulation of auto-neg?
> >>>
> >>> Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
> >>> (BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
> >>> so according to 802.3-2015, we should not be setting 1.5
> >>> (BMSR_ANEGCOMPLETE).
> >>>
> >>> However, swphy does try to emulate autonegotiation - we do have cases
> >>> where swphy is used in situations where the speed and duplex are not
> >>> fixed. It returns an emulated link partner advertisement for the
> >>> current speed, which would suggest that we should set BMCR_ANENABLE.
> >>>
> >> If we emulate auto-neg, then it's not needed to set the speed bits
> >> in BMCR. Also what just comes to my mind, certain speeds like 1000BaseT
> >> don't support forced mode. So we may have to go with auto-neg.
> >
> > Sure.
> >
> >> To avoid the original issue it should be sufficient to copy
> >> supported -> advertising at a suited place.
> >
> Sorry, seems this wasn't clear enough. I don't mean to change
> swphy but the user side.
>
> > Why bother - the software PHY emulation is an emulation to allow
> > existing userspace that pre-dates the ethtool API to get some link
> > parameters. If we augment the PHY emulation in non-standard ways,
> > userspace will need to be updated to handle those non-standard
> > ways. If userspace needs to be updated, why not just bite the
> > bullet and update to ethtool APIs rather than adding more
> > complication through an emulation layer?
> >
> It's not only userspace. Based on my limited knowledge of DSA this
> code also uses e.g. genphy_read_status() with a fixed link.
DSA has support for phylink, which is perfectly capable of supporting
fixed links without using fixed-phy.c, although we have no way to
create that without DT. Support could be added for non-DT though.
That would avoid using phylib functions to read back from a fixed-link
PHY. Since phylink presents to the MAC a fixed link in the same
abstract manner as a real PHY, it should result in a more elegant
implementation.
DSA already has phylink support to support SFPs.
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 12.1Mbps down 622kbps up
According to speedtest.net: 11.9Mbps down 500kbps up
^ permalink raw reply
* Re: [PATCH net] staging: fsl-dpaa2: ethsw: Add missing netdevice check
From: Florian Fainelli @ 2019-02-24 16:17 UTC (permalink / raw)
To: Ioana Ciornei, netdev@vger.kernel.org
Cc: Ioana Ciocoi Radulescu, Greg Kroah-Hartman,
open list:DPAA2 ETHERNET SWITCH DRIVER,
open list:STAGING SUBSYSTEM
In-Reply-To: <VI1PR0402MB28009A8333F247A753F44F94E0780@VI1PR0402MB2800.eurprd04.prod.outlook.com>
Le 2/23/19 à 12:45 AM, Ioana Ciornei a écrit :
>
>> Subject: [PATCH net] staging: fsl-dpaa2: ethsw: Add missing netdevice check
>>
>> port_switchdev_event() does not check that the target network device is
>> actually backed by the ethsw driver, this could be problematic in a stacked
>> environment case.
>>
>
> Just FYI, I sent a patch set containing a similar patch verifying if the netdev is backed by the ethsw:
> https://lkml.org/lkml/2019/2/6/216
>
> I sent the entire patch set against the staging tree.
Thank you. BTW do you have any plans for moving this driver out of
staging? It looks pretty good to me to get promoted in tree, provided
that you also have all the dependencies in place etc.
--
Florian
^ permalink raw reply
* Re: [PATCH 0/3] Cover letter: Add support for high speed MAC in Cadence controller driver
From: Florian Fainelli @ 2019-02-24 16:22 UTC (permalink / raw)
To: Parshuram Thombare, nicolas.ferre, davem, netdev, andrew,
hkallweit1, linux-kernel, rafalc, piotrs, jank
In-Reply-To: <20190222201118.GA14791@lvlogina.cadence.com>
Hi,
Le 2/22/19 à 12:11 PM, Parshuram Thombare a écrit :
> Hello !
>
> This patch series contain changes to support high speed MAC and PCS
> in Cadence ethernet controller driver.
From patch submission perspective, your cover letter and individual
patches do not appear as a reply to this cover letter which makes the
threading broken in most mail clients. If you used git format-patch
--cover-letter, this is handled transparently for you by git send-email.
>
> Here is the list of patches.
> [PATCH 1/3] net: ethernet: add support for PCS and 2.5G speed
> This patch add support for PCS (for SGMII interface) and 2.5Gbps
> MAC in Cadence ethernet controller driver.
> [PATCH 2/3] net: ethernet: add c45 PHY support in MDIO read/write functions.
> This patch modify MDIO read/write functions to support
> communication with C45 PHY in Cadence ethernet controller driver.
> [PATCH 3/3] net: ethernet: add support for high speed mac and usxgmii pcs
> This patch add support for (10G) high speed mac, usxgmii pcs
> and usxgmii interface in Cadence ethernet controller driver.
>
> Regards,
> Parshuram Thombare
>
> Parshuram Thombare (3):
> net: ethernet: add support for PCS and 2.5G speed
> net: ethernet: add c45 PHY support in MDIO read/write functions.
> net: ethernet: add support for high speed mac and usxgmii pcs
>
> drivers/net/ethernet/cadence/macb.h | 68 ++++++-
> drivers/net/ethernet/cadence/macb_main.c | 309 +++++++++++++++++++++++++----
> include/linux/phy.h | 3 +
> 3 files changed, 333 insertions(+), 47 deletions(-)
>
--
Florian
^ permalink raw reply
* Re: No traffic with Marvell switch and latest linux-next
From: Florian Fainelli @ 2019-02-24 16:32 UTC (permalink / raw)
To: Russell King - ARM Linux admin, Heiner Kallweit
Cc: Andrew Lunn, netdev@vger.kernel.org
In-Reply-To: <20190224154900.jmjuh4uz2lvtankg@shell.armlinux.org.uk>
Le 2/24/19 à 7:49 AM, Russell King - ARM Linux admin a écrit :
> On Sun, Feb 24, 2019 at 04:39:30PM +0100, Heiner Kallweit wrote:
>> On 24.02.2019 16:34, Russell King - ARM Linux admin wrote:
>>> On Sun, Feb 24, 2019 at 04:28:32PM +0100, Heiner Kallweit wrote:
>>>> On 24.02.2019 16:15, Russell King - ARM Linux admin wrote:
>>>>> On Sun, Feb 24, 2019 at 04:04:03PM +0100, Andrew Lunn wrote:
>>>>>>> I think what's not correct is that phydev->autoneg is set
>>>>>>> (by phy_device_create) for a fixed link.
>>>>>>
>>>>>> Fixed-link tries to emulate auto-neg:
>>>>>>
>>>>>> bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
>>>>>>
>>>>>> Maybe it needs better emulation of auto-neg?
>>>>>
>>>>> Or maybe it needs to represent a fixed-speed PHY by clearing bit 1.3
>>>>> (BMSR_ANEGCAPABLE). In any case, 0.12 (BMCR_ANENABLE) is not set,
>>>>> so according to 802.3-2015, we should not be setting 1.5
>>>>> (BMSR_ANEGCOMPLETE).
>>>>>
>>>>> However, swphy does try to emulate autonegotiation - we do have cases
>>>>> where swphy is used in situations where the speed and duplex are not
>>>>> fixed. It returns an emulated link partner advertisement for the
>>>>> current speed, which would suggest that we should set BMCR_ANENABLE.
>>>>>
>>>> If we emulate auto-neg, then it's not needed to set the speed bits
>>>> in BMCR. Also what just comes to my mind, certain speeds like 1000BaseT
>>>> don't support forced mode. So we may have to go with auto-neg.
>>>
>>> Sure.
>>>
>>>> To avoid the original issue it should be sufficient to copy
>>>> supported -> advertising at a suited place.
>>>
>> Sorry, seems this wasn't clear enough. I don't mean to change
>> swphy but the user side.
>>
>>> Why bother - the software PHY emulation is an emulation to allow
>>> existing userspace that pre-dates the ethtool API to get some link
>>> parameters. If we augment the PHY emulation in non-standard ways,
>>> userspace will need to be updated to handle those non-standard
>>> ways. If userspace needs to be updated, why not just bite the
>>> bullet and update to ethtool APIs rather than adding more
>>> complication through an emulation layer?
>>>
>> It's not only userspace. Based on my limited knowledge of DSA this
>> code also uses e.g. genphy_read_status() with a fixed link.
>
> DSA has support for phylink, which is perfectly capable of supporting
> fixed links without using fixed-phy.c, although we have no way to
> create that without DT. Support could be added for non-DT though.
I had a branch at some point that provided feature parity with what DT
registration is capable of doing, including representing links between
switches etc [1]. I am not entirely sure it makes to support such a
configuration for platform devices, since they are not so many these
days that would not use DT (except x86 maybe?). It would make sense to
support ACPI-based registration for PHYLINK, if there was a standard for
describing PHYs, SFP/SFFs which is not apparently the case.
[1]: https://github.com/ffainelli/linux/commits/dsa-lbk-pdata2
>
> That would avoid using phylib functions to read back from a fixed-link
> PHY. Since phylink presents to the MAC a fixed link in the same
> abstract manner as a real PHY, it should result in a more elegant
> implementation.
>
> DSA already has phylink support to support SFPs.
>
The added difficulty here and the reason why Andrew went with the
approach that is used by the code currently is because neither do the
CPU or DSA ports are backed by a net_device. It is somewhere on my TODO
to permit the use of PHYLINK without the need of a net_device to cover
those specific DSA cases unless we just brute force the whole thing and
allocate a net_device structure but not register that net_device? Yes in
fact, why don't we do that?
--
Florian
^ permalink raw reply
* [PATCH net-next] net: phy: improve auto-neg emulation in swphy
From: Heiner Kallweit @ 2019-02-24 16:41 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller,
Russell King - ARM Linux
Cc: netdev@vger.kernel.org
Auto-neg emulation currently doesn't set bit BMCR_ANENABLE in BMCR,
add this. Users will ignore speed and duplex settings in BMCR because
we're emulating auto-neg, therefore we can remove related code.
See also following discussion [0].
[0] https://marc.info/?t=155041784900002&r=1&w=2
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/swphy.c | 10 +---------
1 file changed, 1 insertion(+), 9 deletions(-)
diff --git a/drivers/net/phy/swphy.c b/drivers/net/phy/swphy.c
index b9743569e..dad22481d 100644
--- a/drivers/net/phy/swphy.c
+++ b/drivers/net/phy/swphy.c
@@ -19,7 +19,6 @@
#define MII_REGS_NUM 29
struct swmii_regs {
- u16 bmcr;
u16 bmsr;
u16 lpa;
u16 lpagb;
@@ -40,16 +39,13 @@ enum {
*/
static const struct swmii_regs speed[] = {
[SWMII_SPEED_10] = {
- .bmcr = BMCR_FULLDPLX,
.lpa = LPA_10FULL | LPA_10HALF,
},
[SWMII_SPEED_100] = {
- .bmcr = BMCR_FULLDPLX | BMCR_SPEED100,
.bmsr = BMSR_100FULL | BMSR_100HALF,
.lpa = LPA_100FULL | LPA_100HALF,
},
[SWMII_SPEED_1000] = {
- .bmcr = BMCR_FULLDPLX | BMCR_SPEED1000,
.bmsr = BMSR_ESTATEN,
.lpagb = LPA_1000FULL | LPA_1000HALF,
},
@@ -57,13 +53,11 @@ static const struct swmii_regs speed[] = {
static const struct swmii_regs duplex[] = {
[SWMII_DUPLEX_HALF] = {
- .bmcr = ~BMCR_FULLDPLX,
.bmsr = BMSR_ESTATEN | BMSR_100HALF,
.lpa = LPA_10HALF | LPA_100HALF,
.lpagb = LPA_1000HALF,
},
[SWMII_DUPLEX_FULL] = {
- .bmcr = ~0,
.bmsr = BMSR_ESTATEN | BMSR_100FULL,
.lpa = LPA_10FULL | LPA_100FULL,
.lpagb = LPA_1000FULL,
@@ -118,7 +112,6 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
{
int speed_index, duplex_index;
u16 bmsr = BMSR_ANEGCAPABLE;
- u16 bmcr = 0;
u16 lpagb = 0;
u16 lpa = 0;
@@ -136,7 +129,6 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
if (state->link) {
bmsr |= BMSR_LSTATUS | BMSR_ANEGCOMPLETE;
- bmcr |= speed[speed_index].bmcr & duplex[duplex_index].bmcr;
lpa |= speed[speed_index].lpa & duplex[duplex_index].lpa;
lpagb |= speed[speed_index].lpagb & duplex[duplex_index].lpagb;
@@ -149,7 +141,7 @@ int swphy_read_reg(int reg, const struct fixed_phy_status *state)
switch (reg) {
case MII_BMCR:
- return bmcr;
+ return BMCR_ANENABLE;
case MII_BMSR:
return bmsr;
case MII_PHYSID1:
--
2.20.1
Andr
^ permalink raw reply related
* Re: [PATCH] Documentation: networking: switchdev: Update port parent ID section
From: Florian Fainelli @ 2019-02-24 16:46 UTC (permalink / raw)
To: David Miller; +Cc: netdev, andrew, vivien.didelot, idosch, jiri
In-Reply-To: <20190222.115839.1542447665920027412.davem@davemloft.net>
Le 2/22/19 à 11:58 AM, David Miller a écrit :
> From: Florian Fainelli <f.fainelli@gmail.com>
> Date: Wed, 20 Feb 2019 14:58:50 -0800
>
>> Update the section about switchdev drivers having to implement a
>> switchdev_port_attr_get() function to return
>> SWITCHDEV_ATTR_ID_PORT_PARENT_ID since that is no longer valid after
>> commit bccb30254a4a ("net: Get rid of
>> SWITCHDEV_ATTR_ID_PORT_PARENT_ID").
>>
>> Fixes: bccb30254a4a ("net: Get rid of SWITCHDEV_ATTR_ID_PORT_PARENT_ID")
>> Reviewed-by: Ido Schimmel <idosch@mellanox.com>
>> Acked-by: Jiri Pirko <jiri@mellanox.com>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>
> Applied.
>
Looks like you applied this to the "net" tree, and I missed prefixing
the patch to indicate this was targeted at the "net-next" tree. Are you
planning a merge from "net" to "net-next" anytime soon?
Thanks!
--
Florian
^ permalink raw reply
* Re: [PATCH net-next 7/8] net: switchdev: Replace port attr set SDO with a notification
From: Florian Fainelli @ 2019-02-24 16:47 UTC (permalink / raw)
To: Ido Schimmel
Cc: netdev@vger.kernel.org, David S. Miller, open list,
open list:STAGING SUBSYSTEM, moderated list:ETHERNET BRIDGE,
Jiri Pirko, andrew@lunn.ch, vivien.didelot@gmail.com
In-Reply-To: <20190223103207.GA13070@splinter>
Le 2/23/19 à 2:32 AM, Ido Schimmel a écrit :
> On Fri, Feb 22, 2019 at 03:59:25PM -0800, Florian Fainelli wrote:
>> Drop switchdev_ops.switchdev_port_attr_set. Drop the uses of this field
>> from all clients, which were migrated to use switchdev notification in
>> the previous patches.
>>
>> Add a new function switchdev_port_attr_notify() that sends the switchdev
>> notifications SWITCHDEV_PORT_ATTR_SET and takes care, depending on
>> SWITCHDEV_F_DEFER to call the blocking (process) or non-blocking
>> (atomic) notifier chain accordingly.
>>
>> Drop __switchdev_port_attr_set() and update switchdev_port_attr_set()
>> likewise.
>>
>> Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
>> ---
>> net/switchdev/switchdev.c | 96 +++++++++++----------------------------
>> 1 file changed, 26 insertions(+), 70 deletions(-)
>>
>> diff --git a/net/switchdev/switchdev.c b/net/switchdev/switchdev.c
>> index 94400f5b8e07..a1f16836ef89 100644
>> --- a/net/switchdev/switchdev.c
>> +++ b/net/switchdev/switchdev.c
>> @@ -174,81 +174,35 @@ static int switchdev_deferred_enqueue(struct net_device *dev,
>> return 0;
>> }
>>
>> -/**
>> - * switchdev_port_attr_get - Get port attribute
>
> Hmm, why do you remove it here? Can't you remove it in a separate patch?
> I thought we already got rid of it :p
Yes it should have been removed, looks like my previous series did not
that, I will send that separately.
>
>> - *
>> - * @dev: port device
>> - * @attr: attribute to get
>> - */
>> -int switchdev_port_attr_get(struct net_device *dev, struct switchdev_attr *attr)
>> +static int switchdev_port_attr_notify(enum switchdev_notifier_type nt,
>> + struct net_device *dev,
>> + const struct switchdev_attr *attr,
>> + struct switchdev_trans *trans)
>> {
>> - const struct switchdev_ops *ops = dev->switchdev_ops;
>> - struct net_device *lower_dev;
>> - struct list_head *iter;
>> - struct switchdev_attr first = {
>> - .id = SWITCHDEV_ATTR_ID_UNDEFINED
>> - };
>> - int err = -EOPNOTSUPP;
>> + int err;
>> + int rc;
>>
>> - if (ops && ops->switchdev_port_attr_get)
>> - return ops->switchdev_port_attr_get(dev, attr);
>> + struct switchdev_notifier_port_attr_info attr_info = {
>> + .attr = attr,
>> + .trans = trans,
>> + .handled = false,
>> + };
>>
>> - if (attr->flags & SWITCHDEV_F_NO_RECURSE)
>> + if (attr & SWITCHDEV_F_DEFER)
>> + rc = call_switchdev_blocking_notifiers(nt, dev,
>> + &attr_info.info, NULL);
>> + else
>> + rc = call_switchdev_notifiers(nt, dev, &attr_info.info, NULL);
>
> I don't believe this is needed. You're calling this function from
> switchdev_port_attr_set_now() which is always called from process
> context. switchdev_port_attr_set() takes care of that. Similar to
> switchdev_port_obj_add().
Except for net/bridge/br_switchdev.c when we check the bridge port's
flags support with PRE_BRIDGE_FLAGS. In that case we are executing from
the caller (atomic) context and we can't defer otherwise that trumps the
whole idea of being able to do a quick check and return that to the
caller that we cannot support specific flags. How would you recommend
approaching that?
--
Florian
^ permalink raw reply
* Re: [PATCH ethtool] ethtool: Add support for 200Gbps (50Gbps per lane) link mode
From: Andrew Lunn @ 2019-02-24 16:47 UTC (permalink / raw)
To: Tariq Toukan; +Cc: John W. Linville, netdev, Eran Ben Elisha, Aya Levin
In-Reply-To: <1551020901-20257-1-git-send-email-tariqt@mellanox.com>
On Sun, Feb 24, 2019 at 05:08:21PM +0200, Tariq Toukan wrote:
> From: Aya Levin <ayal@mellanox.com>
> index 5a26cff5fb33..64ce0711ad5f 100644
> --- a/ethtool.8.in
> +++ b/ethtool.8.in
> @@ -650,6 +650,11 @@ lB l lB.
> 0x400000000 50000baseCR2 Full
> 0x800000000 50000baseKR2 Full
> 0x10000000000 50000baseSR2 Full
> +0x10000000000000 50000baseKR Full
> +0x20000000000000 50000baseSR Full
> +0x40000000000000 50000baseCR Full
> +0x80000000000000 50000baseLR_ER_FR Full
> +0x100000000000000 50000baseDR Full
> 0x8000000 56000baseKR4 Full
> 0x10000000 56000baseCR4 Full
> 0x20000000 56000baseSR4 Full
> @@ -658,6 +663,16 @@ lB l lB.
> 0x2000000000 100000baseSR4 Full
> 0x4000000000 100000baseCR4 Full
> 0x8000000000 100000baseLR4_ER4 Full
> +0x200000000000000 100000baseKR2 Full
> +0x400000000000000 100000baseSR2 Full
> +0x800000000000000 100000baseCR2 Full
> +0x1000000000000000 100000baseLR2_ER2_FR2 Full
> +0x2000000000000000 100000baseDR2 Full
> +0x4000000000000000 200000baseKR4 Full
> +0x8000000000000000 200000baseSR4 Full
> +0x10000000000000000 200000baseLR4_ER4_FR4 Full
> +0x20000000000000000 200000baseDR4 Full
> +0x40000000000000000 200000baseCR4 Full
This is getting less friendly all the time, and it was never very
friendly to start with. We have the strings which represent these link
modes in the table used for dumping caps. How about allowing the user
to list a comma separate list of modes.
ethtool -s lan42 advertise 100000baseKR2/Full,100000baseSR2/Full,100000baseCR2/Full
Andrew
> + adv_bit = ETHTOOL_LINK_MODE_200000baseKR4_Full_BIT &
> + ETHTOOL_LINK_MODE_200000baseSR4_Full_BIT &
> + ETHTOOL_LINK_MODE_200000baseLR4_ER4_FR4_Full_BIT &
> + ETHTOOL_LINK_MODE_200000baseDR4_Full_BIT &
> + ETHTOOL_LINK_MODE_200000baseCR4_Full_BIT;
Maybe i'm wrong, but this looks odd.
enum ethtool_link_mode_bit_indices {
ETHTOOL_LINK_MODE_10baseT_Half_BIT = 0,
ETHTOOL_LINK_MODE_10baseT_Full_BIT = 1,
ETHTOOL_LINK_MODE_100baseT_Half_BIT = 2,
ETHTOOL_LINK_MODE_100baseT_Full_BIT = 3,
ETHTOOL_LINK_MODE_1000baseT_Half_BIT = 4,
ETHTOOL_LINK_MODE_1000baseT_Full_BIT = 5,
These are numbers, not bitmasks, so & them together does not look
correct.
Andrew
^ permalink raw reply
* [PATCH net] net: phy: fix reading fixed phy status
From: Heiner Kallweit @ 2019-02-24 17:01 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev@vger.kernel.org
With the switch to phy_resolve_aneg_linkmode() we don't read from the
chip any longer what is advertised but use phydev->advertising directly.
For a fixed phy however this bitmap is empty so far, what results in
no common mode being found. This breaks DSA. Fix this by advertising
everything that is supported. For a normal phy this done by phy_probe().
Fixes: 5502b218e001 ("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
---
drivers/net/phy/fixed_phy.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/phy/fixed_phy.c b/drivers/net/phy/fixed_phy.c
index b0d1368c3..1acd8bfdb 100644
--- a/drivers/net/phy/fixed_phy.c
+++ b/drivers/net/phy/fixed_phy.c
@@ -22,6 +22,7 @@
#include <linux/seqlock.h>
#include <linux/idr.h>
#include <linux/netdevice.h>
+#include <linux/linkmode.h>
#include "swphy.h"
@@ -300,6 +301,8 @@ static struct phy_device *__fixed_phy_register(unsigned int irq,
phy->supported);
}
+ linkmode_copy(phy->advertising, phy->supported);
+
ret = phy_device_register(phy);
if (ret) {
phy_device_free(phy);
--
2.20.1
^ permalink raw reply related
* Re: No traffic with Marvell switch and latest linux-next
From: Andrew Lunn @ 2019-02-24 17:04 UTC (permalink / raw)
To: Florian Fainelli
Cc: Russell King - ARM Linux admin, Heiner Kallweit,
netdev@vger.kernel.org
In-Reply-To: <7005686d-45cb-a8af-de34-873c9e34a021@gmail.com>
> The added difficulty here and the reason why Andrew went with the
> approach that is used by the code currently is because neither do the
> CPU or DSA ports are backed by a net_device. It is somewhere on my TODO
> to permit the use of PHYLINK without the need of a net_device to cover
> those specific DSA cases unless we just brute force the whole thing and
> allocate a net_device structure but not register that net_device? Yes in
> fact, why don't we do that?
Hi Florian
At the moment, we are using a phydev which is not connected to a
MAC. That is rather odd, but the phylib maintainers mostly know about
this, and keep an eye out for changes which might break any
assumptions. And the phylib API is quite small.
How many assumptions are going to break with a netdev which is not
registered? The API is much bigger, more people hack on it, and it is
going to be much harder to review changes to make sure assumptions are
not changed.
If we are going to do something odd, we should keep the scope as small
as possible.
Andrew
^ permalink raw reply
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