* Re: [net-next PATCH 3/5] net: netcp: ethss enhancements to support 2u cpsw h/w on K2G SoC
From: Murali Karicheri @ 2018-03-27 13:23 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, ssantosh, malat, w-kwok2, devicetree,
linux-kernel, linux-arm-kernel, davem, netdev
In-Reply-To: <20180326202842.GB5862@lunn.ch>
On 03/26/2018 04:28 PM, Andrew Lunn wrote:
> On Mon, Mar 26, 2018 at 04:15:10PM -0400, Murali Karicheri wrote:
>> K2G SoC uses 2u cpsw h/w. It uses RGMII instead of SGMII to interface with
>> Phy. This patch enhances the driver to check RGMII status instead of SGMII
>> status for link state determination. Also map all of the vlan priorities
>> to zero as the packet DMA is enabled to receive only flow id 0 which maps
>> to priority zero.
>>
>> Additionally, When a phy with rgmii interface requires internal delay, the
>> same is set in the phy driver. To support such phy devices, add a phy-mode
>> handling code in the driver using of_get_phy_mode() and pass the obtained
>> phy mode to of_phy_connect()
>
> Hi Murali
>
> Please break this patch up. One patch should do one thing. That makes
> it easy to review. There are too many things going on at once here.
>
> Andrew
>
Hello Andrew,
Thanks for the comment. But I am not sure how to break this up as this is
an enhancement to the driver to support a newer version of the cspw
hardware. Without all these pieces together, the driver can't
function. Probably I can make the below break up based on different functions
added. Let me know if this looks good to you. Beware that patch #2 and
#3 are small patches and majority of code change will be in patch #1
which has to go together.
Patch #1. Add support new link interface, RGMII_LINK_MAC_PHY, for K2G
- Most of the code is for this
Patch #2. Add support for configuring phy_mode
- This just add phy_mode handling code
Patch #3. map all vlan priorities to flow id 0
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* Re: [PATCH v3 iproute2-next 1/8] rdma: include rdma-core <rdma/rdma_cma.h>
From: David Ahern @ 2018-03-27 13:23 UTC (permalink / raw)
To: Leon Romanovsky, Steve Wise; +Cc: stephen, netdev, linux-rdma
In-Reply-To: <20180327054622.GU1877@mtr-leonro.local>
On 3/26/18 11:46 PM, Leon Romanovsky wrote:
> On Mon, Mar 26, 2018 at 01:57:32PM -0700, Steve Wise wrote:
>> This avoids requiring rdma-core be installed on systems.
>>
>> Signed-off-by: Steve Wise <swise@opengridcomputing.com>
>> ---
>> rdma/include/rdma/rdma_cma.h | 728 +++++++++++++++++++++++++++++++++++++++++++
>> 1 file changed, 728 insertions(+)
>> create mode 100644 rdma/include/rdma/rdma_cma.h
>>
>
> Steve,
>
> Sorry for not spotting it before, you actually need only 3 enums for the
> cm_id_ps_to_str() from rdma_cma.h.
>
> Simply copy/paste that enum into cm_id_ps_to_str().
>
> Thanks
>
I think this is Jason's point: if that enum is part of the UAPI why
isn't it part of a uapi header file?
We definitely do not want that entire header file brought in to iproute2
^ permalink raw reply
* Re: [PATCH net-next 1/4] qed: Populate nvm image attribute shadow.
From: Yuval Mintz @ 2018-03-27 13:01 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru; +Cc: davem, netdev, Ariel.Elior
In-Reply-To: <20180326101348.21075-2-sudarsana.kalluru@cavium.com>
On Mon, Mar 26, 2018 at 03:13:45AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch add support for populating the flash image attributes.
s/add/adds/
[...]
> -int qed_mcp_bist_nvm_test_get_image_att(struct qed_hwfn *p_hwfn,
> - struct qed_ptt *p_ptt,
> - struct bist_nvm_image_att *p_image_att,
> +int qed_mcp_bist_nvm_get_image_att(struct qed_hwfn *p_hwfn,
> + struct qed_ptt *p_ptt,
> + struct bist_nvm_image_att *p_image_att,
> u32 image_index)
Indentation seems broken.
>
> +int qed_mcp_nvm_info_populate(struct qed_hwfn *p_hwfn)
> +{
> + struct qed_nvm_image_info *nvm_info = &p_hwfn->nvm_info;
> + struct qed_ptt *p_ptt;
> + int rc;
> + u32 i;
> +
> + p_ptt = qed_ptt_acquire(p_hwfn);
> + if (!p_ptt) {
> + DP_ERR(p_hwfn, "failed to acquire ptt\n");
> + return -EBUSY;
> + }
> +
> + /* Acquire from MFW the amount of available images */
> + nvm_info->num_images = 0;
> + rc = qed_mcp_bist_nvm_get_num_images(p_hwfn,
> + p_ptt, &nvm_info->num_images);
> + if (rc == -EOPNOTSUPP) {
> + DP_INFO(p_hwfn, "DRV_MSG_CODE_BIST_TEST is not supported\n");
> + goto out;
> + } else if ((rc != 0) || (nvm_info->num_images == 0)) {
rc || !nvm_info->num_images
> + DP_ERR(p_hwfn, "Failed getting number of images\n");
> + goto err0;
> + }
> +
> + nvm_info->image_att =
> + kmalloc(nvm_info->num_images * sizeof(struct bist_nvm_image_att),
> + GFP_KERNEL);
Indentation can be better than this.
[...]
> --- a/drivers/net/ethernet/qlogic/qed/qed_selftest.c
> +++ b/drivers/net/ethernet/qlogic/qed/qed_selftest.c
> @@ -125,10 +125,11 @@ int qed_selftest_nvram(struct qed_dev *cdev)
> }
>
> /* Acquire from MFW the amount of available images */
> - rc = qed_mcp_bist_nvm_test_get_num_images(p_hwfn, p_ptt, &num_images);
> + rc = qed_mcp_bist_nvm_get_num_images(p_hwfn, p_ptt, &num_images);
> if (rc || !num_images) {
> DP_ERR(p_hwfn, "Failed getting number of images\n");
> - return -EINVAL;
> + rc = -EINVAL;
> + goto err0;
Well, this one is a bug fix [Failure flow currently leaks a PTT entry].
If you don't want to treat it as one that's fine, but I think it
deserves its own patch in the series.
> }
>
> /* Iterate over images and validate CRC */
> @@ -136,8 +137,8 @@ int qed_selftest_nvram(struct qed_dev *cdev)
> /* This mailbox returns information about the image required for
> * reading it.
> */
> - rc = qed_mcp_bist_nvm_test_get_image_att(p_hwfn, p_ptt,
> - &image_att, i);
> + rc = qed_mcp_bist_nvm_get_image_att(p_hwfn, p_ptt,
> + &image_att, i);
> if (rc) {
> DP_ERR(p_hwfn,
> "Failed getting image index %d attributes\n",
> --
> 1.8.3.1
>
^ permalink raw reply
* Re: [PATCH] net: fec: set dma_coherent_mask
From: Lino Sanfilippo @ 2018-03-27 13:24 UTC (permalink / raw)
To: Greg Ungerer; +Cc: netdev, linux-m68k, Greg Ungerer
In-Reply-To: <1522071386-29743-1-git-send-email-gerg@linux-m68k.org>
Hi,
>
> + dma_set_coherent_mask(&fep->pdev->dev, DMA_BIT_MASK(32));
> +
> for (i = 0; i < fep->num_tx_queues; i++) {
> txq = kzalloc(sizeof(*txq), GFP_KERNEL);
> if (!txq) {
dma_set_coherent_mask() can fail, so the return value should be checked and
a failure be handled accordingly.
Regards,
Lino
^ permalink raw reply
* Re: [PATCH net v2] udp6: set dst cache for a connected sk before udp_v6_send_skb
From: Alexey Kodanev @ 2018-03-27 13:27 UTC (permalink / raw)
To: Martin KaFai Lau; +Cc: netdev, Eric Dumazet, David Miller
In-Reply-To: <20180326170037.36xikwnham3zaiel@kafai-mbp.dhcp.thefacebook.com>
On 26.03.2018 20:02, Martin KaFai Lau wrote:
> On Mon, Mar 26, 2018 at 05:48:47PM +0300, Alexey Kodanev wrote:
>> After commit 33c162a980fe ("ipv6: datagram: Update dst cache of a
>> connected datagram sk during pmtu update"), when the error occurs on
>> sending datagram in udpv6_sendmsg() due to ICMPV6_PKT_TOOBIG type,
>> error handler can trigger the following path and call ip6_dst_store():
>>
>> udpv6_err()
>> ip6_sk_update_pmtu()
>> ip6_datagram_dst_update()
>> ip6_dst_lookup_flow(), can create a RTF_CACHE clone
> Instead of ip6_dst_lookup_flow(),
> you meant the RTF_CACHE route created in ip6_update_pmtu()
>
Right, or even earlier... I was using vti tunnel and it invokes
skb_dst_update_pmtu() on this error, then sends ICMPv6_PKT_TOOBIG.
>> ...
>> ip6_dst_store()
>>
>> It can happen before a connected UDP socket invokes ip6_dst_store()
>> in the end of udpv6_sendmsg(), on destination release, as a result,
>> the last one changes dst to the old one, preventing getting updated
>> dst cache on the next udpv6_sendmsg() call.
>>
>> This patch moves ip6_dst_store() in udpv6_sendmsg(), so that it is
>> invoked after ip6_sk_dst_lookup_flow() and before udp_v6_send_skb().
> After this patch, the above udpv6_err() path could not happen after
> ip6_sk_dst_lookup_flow() and before the ip6_dst_store() in udpv6_sendmsg()?
>
May be we could minimize this if save it in ip6_sk_dst_lookup_flow()
for a connected UDP sockets only if we're not getting it from a cache
for some reason?
diff --git a/net/ipv6/ip6_output.c b/net/ipv6/ip6_output.c
index a8a9195..0204f52 100644
--- a/net/ipv6/ip6_output.c
+++ b/net/ipv6/ip6_output.c
@@ -1115,13 +1115,30 @@ struct dst_entry *ip6_dst_lookup_flow(const struct sock *sk, struct flowi6 *fl6,
* error code.
*/
struct dst_entry *ip6_sk_dst_lookup_flow(struct sock *sk, struct flowi6 *fl6,
- const struct in6_addr *final_dst)
+ const struct in6_addr *final_dst,
+ bool connected)
{
struct dst_entry *dst = sk_dst_check(sk, inet6_sk(sk)->dst_cookie);
dst = ip6_sk_dst_check(sk, dst, fl6);
- if (!dst)
- dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
+ if (dst)
+ return dst;
+
+ dst = ip6_dst_lookup_flow(sk, fl6, final_dst);
+
+ if (connected && !IS_ERR(dst))
+ ip6_dst_store(sk, dst_clone(dst), ...);
Thanks,
Alexey
>>
>> Also, increase refcnt for dst, when passing it to ip6_dst_store()
>> because after that the dst cache can be released by other calls
>> to ip6_dst_store() with the same socket.
>>
>> Fixes: 33c162a980fe ("ipv6: datagram: Update dst cache of a connected datagram sk during pmtu update")
>> Signed-off-by: Alexey Kodanev <alexey.kodanev@oracle.com>
>> ---
>>
>> v2: * remove 'release_dst:' label
>>
>> * move ip6_dst_store() below MSG_CONFIRM check as
>> suggested by Eric and add dst_clone()
>>
>> * add 'Fixes' commit.
>>
>>
>> net/ipv6/udp.c | 29 +++++++++++------------------
>> 1 file changed, 11 insertions(+), 18 deletions(-)
>>
>> diff --git a/net/ipv6/udp.c b/net/ipv6/udp.c
>> index 52e3ea0..4508e5a 100644
>> --- a/net/ipv6/udp.c
>> +++ b/net/ipv6/udp.c
>> @@ -1303,6 +1303,16 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>> goto do_confirm;
>> back_from_confirm:
>>
>> + if (connected)>> + ip6_dst_store(sk, dst_clone(dst),
>> + ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
>> + &sk->sk_v6_daddr : NULL,
>> +#ifdef CONFIG_IPV6_SUBTREES
>> + ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
>> + &np->saddr :
>> +#endif
>> + NULL);
>> +
>> /* Lockless fast path for the non-corking case */
>> if (!corkreq) {
>> struct sk_buff *skb;
>> @@ -1314,7 +1324,7 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>> err = PTR_ERR(skb);
>> if (!IS_ERR_OR_NULL(skb))
>> err = udp_v6_send_skb(skb, &fl6);
>> - goto release_dst;
>> + goto out;
>> }
>>
>> lock_sock(sk);
>> @@ -1348,23 +1358,6 @@ int udpv6_sendmsg(struct sock *sk, struct msghdr *msg, size_t len)
>> err = np->recverr ? net_xmit_errno(err) : 0;
>> release_sock(sk);
>>
>> -release_dst:
>> - if (dst) {
>> - if (connected) {
>> - ip6_dst_store(sk, dst,
>> - ipv6_addr_equal(&fl6.daddr, &sk->sk_v6_daddr) ?
>> - &sk->sk_v6_daddr : NULL,
>> -#ifdef CONFIG_IPV6_SUBTREES
>> - ipv6_addr_equal(&fl6.saddr, &np->saddr) ?
>> - &np->saddr :
>> -#endif
>> - NULL);
>> - } else {
>> - dst_release(dst);
>> - }
>> - dst = NULL;
>> - }
>> -
>> out:
>> dst_release(dst);
>> fl6_sock_release(flowlabel);
>> --
>> 1.8.3.1
>>
^ permalink raw reply related
* Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
From: Murali Karicheri @ 2018-03-27 13:32 UTC (permalink / raw)
To: Andrew Lunn
Cc: robh+dt, mark.rutland, ssantosh, malat, w-kwok2, devicetree,
linux-kernel, linux-arm-kernel, davem, netdev
In-Reply-To: <20180326204802.GC5862@lunn.ch>
Hello Andrew,
On 03/26/2018 04:48 PM, Andrew Lunn wrote:
> On Mon, Mar 26, 2018 at 04:15:09PM -0400, Murali Karicheri wrote:
>> This patch provide APIs to allow client drivers to support
>> probe deferral. On K2G SoC, devices can be probed only
>> after the ti_sci_pm_domains driver is probed and ready.
>> As drivers may get probed at different order, any driver
>> that depends on knav dma and qmss drivers, for example
>> netcp network driver, needs to defer probe until
>> knav devices are probed and ready to service. To do this,
>> add an API to query the device ready status from the knav
>> dma and qmss devices.
>
> Hi Murali
>
> Shouldn't you really re-write this to be a dma driver? You would then
> do something like of_dma_request_slave_channel() in the ethernet
> driver probe function. That probably correctly returns EPROBE_DEFER.
>
> Andrew
>
Could you please elaborate? These knav dma and qmss drivers are
introduced to support packet DMA hardware available in Keystone
NetCP which couldn't be implemented using the DMA APIs available
at the time this driver was introduced. Another reason was that
the performance was really bad. We had an internal implementation
based on DMA API before which couldn't be upstreamed at that time
due to the reason that we were mis-using the API for this driver.
So we introduced these knav_dma driver to support NetCP. We don't
have any plan to re-write the driver at this time.
If your question is about EPROBE_DEFER being returned from an
existing knav_dma API and using the return code to achieve probe
defer instead of introducing these APIs, I can take a look into
that and respond. So please clarify.
--
Murali Karicheri
Linux Kernel, Keystone
^ permalink raw reply
* [PATCH v2 net 1/1] qede: Fix barrier usage after tx doorbell write.
From: Manish Chopra @ 2018-03-27 13:34 UTC (permalink / raw)
To: davem; +Cc: netdev, ariel.elior, michal.kalderon
Since commit c5ad119fb6c09b0297446be05bd66602fa564758
("net: sched: pfifo_fast use skb_array") driver is exposed
to an issue where it is hitting NULL skbs while handling TX
completions. Driver uses mmiowb() to flush the writes to the
doorbell bar which is a write-combined bar, however on x86
mmiowb() does not flush the write combined buffer.
This patch fixes this problem by replacing mmiowb() with wmb()
after the write combined doorbell write so that writes are
flushed and synchronized from more than one processor.
V1->V2:
-------
This patch was marked as "superseded" in patchwork.
(Not really sure for what reason).Resending it as v2.
Signed-off-by: Ariel Elior <ariel.elior@cavium.com>
Signed-off-by: Manish Chopra <manish.chopra@cavium.com>
---
drivers/net/ethernet/qlogic/qede/qede_fp.c | 10 ++++------
1 files changed, 4 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_fp.c b/drivers/net/ethernet/qlogic/qede/qede_fp.c
index dafc079..2e921ca 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_fp.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_fp.c
@@ -320,13 +320,11 @@ static inline void qede_update_tx_producer(struct qede_tx_queue *txq)
barrier();
writel(txq->tx_db.raw, txq->doorbell_addr);
- /* mmiowb is needed to synchronize doorbell writes from more than one
- * processor. It guarantees that the write arrives to the device before
- * the queue lock is released and another start_xmit is called (possibly
- * on another CPU). Without this barrier, the next doorbell can bypass
- * this doorbell. This is applicable to IA64/Altix systems.
+ /* Fence required to flush the write combined buffer, since another
+ * CPU may write to the same doorbell address and data may be lost
+ * due to relaxed order nature of write combined bar.
*/
- mmiowb();
+ wmb();
}
static int qede_xdp_xmit(struct qede_dev *edev, struct qede_fastpath *fp,
--
1.7.1
^ permalink raw reply related
* Re: [PATCH net-next 3/4] qed: Adapter flash update support.
From: Yuval Mintz @ 2018-03-27 13:36 UTC (permalink / raw)
To: Sudarsana Reddy Kalluru; +Cc: davem, netdev, Ariel.Elior
In-Reply-To: <20180326101348.21075-4-sudarsana.kalluru@cavium.com>
On Mon, Mar 26, 2018 at 03:13:47AM -0700, Sudarsana Reddy Kalluru wrote:
> This patch adds the required driver support for updating the flash or
> non volatile memory of the adapter. At highlevel, flash upgrade comprises
> of reading the flash images from the input file, validating the images and
> writing it to the respective paritions.
s/it/them/
[...]
> + * /----------------------------------------------------------------------\
> + * 0B | 0x4 [command index] |
> + * 4B | image_type | Options | Number of register settings |
> + * 8B | Value |
> + * 12B | Mask |
> + * 16B | Offset |
> + * \----------------------------------------------------------------------/
> + * There can be several Value-Mask-Offset sets as specified by 'Number of...'.
> + * Options - 0'b - Calculate & Update CRC for image
> + */
> +static int qed_nvm_flash_image_access(struct qed_dev *cdev, const u8 **data,
> + bool *check_resp)
> +{
> + struct qed_nvm_image_att nvm_image;
> + struct qed_hwfn *p_hwfn;
> + bool is_crc = false;
> + u32 image_type;
> + int rc = 0, i;
> + u16 len;
> +
+
> + nvm_image.start_addr = p_hwfn->nvm_info.image_att[i].nvm_start_addr;
> + nvm_image.length = p_hwfn->nvm_info.image_att[i].len;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "Read image %02x; type = %08x; NVM [%08x,...,%08x]\n",
> + **data, nvm_image.start_addr, image_type,
> + nvm_image.start_addr + nvm_image.length - 1);
Looks like 3rd and 4th printed parameters are flipped.
> + (*data)++;
> + is_crc = !!(**data);
If you'd actually want to be able to use the reserved bits
[forward-compatibility] in the future, you should check bit 0 instead of
checking the byte.
> + (*data)++;
> + len = *((u16 *)*data);
> + *data += 2;
[...]
> +
> +/* Binary file format -
> + * /----------------------------------------------------------------------\
> + * 0B | 0x3 [command index] |
> + * 4B | b'0: check_response? | b'1-127 reserved |
This shows there are 128 bits in a 4 byte field.
> + * 8B | File-type | reserved |
> + * \----------------------------------------------------------------------/
> + * Start a new file of the provided type
> + */
> +static int qed_nvm_flash_image_file_start(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp)
> +{
> + int rc;
> +
> + *data += 4;
> + *check_resp = !!(**data);
Like above
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to start a new file of type %02x\n", **data);
> + rc = qed_mcp_nvm_put_file_begin(cdev, **data);
> + *data += 4;
> +
> + return rc;
> +}
> +
> +/* Binary file format -
> + * /----------------------------------------------------------------------\
> + * 0B | 0x2 [command index] |
> + * 4B | Length in bytes |
> + * 8B | b'0: check_response? | b'1-127 reserved |
Same as above
> + * 12B | Offset in bytes |
> + * 16B | Data ... |
> + * \----------------------------------------------------------------------/
> + * Write data as part of a file that was previously started. Data should be
> + * of length equal to that provided in the message
> + */
> +static int qed_nvm_flash_image_file_data(struct qed_dev *cdev,
> + const u8 **data, bool *check_resp)
> +{
> + u32 offset, len;
> + int rc;
> +
> + *data += 4;
> + len = *((u32 *)(*data));
> + *data += 4;
> + *check_resp = !!(**data);
Same as above
> + *data += 4;
> + offset = *((u32 *)(*data));
> + *data += 4;
> +
> + DP_VERBOSE(cdev, NETIF_MSG_DRV,
> + "About to write File-data: %08x bytes to offset %08x\n",
> + len, offset);
> +
> + rc = qed_mcp_nvm_write(cdev, QED_PUT_FILE_DATA, offset,
> + (char *)(*data), len);
> + *data += len;
> +
> + return rc;
> +}
[...]
> +
> +static int qed_nvm_flash(struct qed_dev *cdev, const char *name)
> +{
> + rc = qed_nvm_flash_image_validate(cdev, image, &data);
> + if (rc)
> + goto exit;
> +
> + while (data < data_end) {
> + bool check_resp = false;
> +
> + /* Parse the actual command */
> + cmd_type = *((u32 *)data);
What's the final format of the file? Is it LE?
> + switch (cmd_type) {
> + case QED_NVM_FLASH_CMD_FILE_DATA:
> + rc = qed_nvm_flash_image_file_data(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_FILE_START:
> + rc = qed_nvm_flash_image_file_start(cdev, &data,
> + &check_resp);
> + break;
> + case QED_NVM_FLASH_CMD_NVM_CHANGE:
> + rc = qed_nvm_flash_image_access(cdev, &data,
> + &check_resp);
> + break;
> + default:
> + DP_ERR(cdev, "Unknown command %08x\n", cmd_type);
> + rc = -EINVAL;
> + break;
Either goto or drop the print; You're getting from the next condition.
> + }
> +
> + if (rc) {
> + DP_ERR(cdev, "Command %08x failed\n", cmd_type);
> + goto exit;
> + }
> +
> + /* Check response if needed */
> + if (check_resp) {
> + u32 mcp_response = 0;
> +
> + if (qed_mcp_nvm_resp(cdev, (u8 *)&mcp_response)) {
> + DP_ERR(cdev, "Failed getting MCP response\n");
> + rc = -EINVAL;
> + goto exit;
> + }
> +
> + switch (mcp_response & FW_MSG_CODE_MASK) {
> + case FW_MSG_CODE_OK:
> + case FW_MSG_CODE_NVM_OK:
> + case FW_MSG_CODE_NVM_PUT_FILE_FINISH_OK:
> + case FW_MSG_CODE_PHY_OK:
> + break;
> + default:
> + DP_ERR(cdev, "MFW returns error: %08x\n",
> + mcp_response);
> + rc = -EINVAL;
> + goto exit;
> + }
> + }
> + }
> +
> +exit:
> + release_firmware(image);
> +
> + return rc;
> +}
> +
^ permalink raw reply
* Re: [PATCH] staging: fsl-dpaa2/ethsw: Fix TCI values overwrite
From: Andrew Lunn @ 2018-03-27 13:37 UTC (permalink / raw)
To: Razvan Stefanescu
Cc: gregkh, devel, linux-kernel, netdev, alexandru.marginean,
ruxandra.radulescu, ioana.ciornei, laurentiu.tudor, stuyoder
In-Reply-To: <20180327131050.30581-1-razvan.stefanescu@nxp.com>
On Tue, Mar 27, 2018 at 08:10:50AM -0500, Razvan Stefanescu wrote:
> Previous implementation overwrites PCP value, assuming the default value is
> 0, instead of 7.
>
> Avoid this by modifying helper function ethsw_port_set_tci() to
> ethsw_port_set_pvid() and make it update only the vlan_id of the tci_cfg
> struct.
Hi Razvan
It is a good idea to explain acronyms, especially for staging, since
there are patches for all sorts of devices, can you cannot expect
everybody to know network specific acronyms.
By PCP you mean Priority Code Point. TCI i have no idea about.
Looking at the code, i think you are changing the flow to become
read/modify/write, instead of just write, which is overwriting the
previously configured Priority Code Point?
Please try to add more details to your change logs, to help us
understand the change.
Andrew
^ permalink raw reply
* Re: [PATCH iproute2-next] tc: Fix compilation error with old iptables
From: David Ahern @ 2018-03-27 13:40 UTC (permalink / raw)
To: Roi Dayan; +Cc: netdev, Stephen Hemminger, Eyal Birger
In-Reply-To: <1522142448-27697-1-git-send-email-roid@mellanox.com>
On 3/27/18 3:20 AM, Roi Dayan wrote:
> The compat_rev field does not exists in old versions of iptables.
> e.g. iptables 1.4.
>
> Fixes: dd29621578d2 ("tc: add em_ipt ematch for calling xtables matches from tc matching context")
> Signed-off-by: Roi Dayan <roid@mellanox.com>
> ---
> tc/em_ipt.c | 2 ++
> 1 file changed, 2 insertions(+)
>
Applied to iproute2-next. Thanks,
^ permalink raw reply
* Re: [net-next PATCH 3/5] net: netcp: ethss enhancements to support 2u cpsw h/w on K2G SoC
From: Andrew Lunn @ 2018-03-27 13:47 UTC (permalink / raw)
To: Murali Karicheri
Cc: robh+dt, mark.rutland, ssantosh, malat, w-kwok2, devicetree,
linux-kernel, linux-arm-kernel, davem, netdev
In-Reply-To: <be58cf50-2148-cbfa-c7a2-8f4fbe6930e5@ti.com>
> Hello Andrew,
>
> Thanks for the comment. But I am not sure how to break this up as this is
> an enhancement to the driver to support a newer version of the cspw
> hardware. Without all these pieces together, the driver can't
> function.
Hi Murali
A few things to consider.
1) You can introduce new features needed by the new hardware one piece
at a time. The new hardware will then work when all the pieces have
been added. This is fine, the new hardware never worked before, so you
are not breaking anything.
2) Your changes could break the old hardware. By having lots of small
changes, you can do a git bissect and find which of the small changes
broke it.
3) It is much easier to review 10 small obviously correct patches than
one huge complex patch, which is not obvious.
> Probably I can make the below break up based on different functions
> added. Let me know if this looks good to you. Beware that patch #2 and
> #3 are small patches and majority of code change will be in patch #1
> which has to go together.
>
> Patch #1. Add support new link interface, RGMII_LINK_MAC_PHY, for K2G
> - Most of the code is for this
> Patch #2. Add support for configuring phy_mode
> - This just add phy_mode handling code
> Patch #3. map all vlan priorities to flow id 0
That sounds better.
Thanks
Andrew
^ permalink raw reply
* Re: [PATCH net] vhost: correctly remove wait queue during poll failure
From: Michael S. Tsirkin @ 2018-03-27 13:58 UTC (permalink / raw)
To: Jason Wang; +Cc: netdev, linux-kernel, kvm, virtualization
In-Reply-To: <b1576bcf-652a-7c1a-9d77-db54931ab979@redhat.com>
On Tue, Mar 27, 2018 at 05:43:14PM +0800, Jason Wang wrote:
>
>
> On 2018年03月27日 17:28, Darren Kenny wrote:
> > Hi Jason,
> >
> > On Tue, Mar 27, 2018 at 11:47:22AM +0800, Jason Wang wrote:
> > > We tried to remove vq poll from wait queue, but do not check whether
> > > or not it was in a list before. This will lead double free. Fixing
> > > this by checking poll->wqh to make sure it was in a list.
> >
> > This text seems at odds with the code below, instead of checking
> > poll-whq, you are removing that check...
> >
> > Maybe the text needs rewording?
>
> Yes, I admit it's bad, thanks for pointing out.
>
> How about:
>
> "Fixing this by switching to use vhost_poll_stop() which zeros poll->wqh
> after removing poll from waitqueue to make sure it won't be freed twice."
>
> Thanks
Let's be a bit more specific about the problem maybe?
when vhost's attempt to start polling a descriptor fails, we remove the
poll->wqh entry from wait queue but do not clear it, so the following
cleanup (e.g. on release) will attempt to remove it again.
To fix, switch to vhost_poll_stop() which zeros poll->wqh
after removing poll from waitqueue to make sure it won't be freed twice."
the patch itself is fine though:
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> >
> > Thanks,
> >
> > Darren.
> >
> > >
> > > Reported-by: syzbot+c0272972b01b872e604a@syzkaller.appspotmail.com
> > > Fixes: 2b8b328b61c79 ("vhost_net: handle polling errors when setting
> > > backend")
> > > Signed-off-by: Jason Wang <jasowang@redhat.com>
> > > ---
> > > drivers/vhost/vhost.c | 3 +--
> > > 1 file changed, 1 insertion(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> > > index 1b3e8d2d..5d5a9d9 100644
> > > --- a/drivers/vhost/vhost.c
> > > +++ b/drivers/vhost/vhost.c
> > > @@ -212,8 +212,7 @@ int vhost_poll_start(struct vhost_poll *poll,
> > > struct file *file)
> > > if (mask)
> > > vhost_poll_wakeup(&poll->wait, 0, 0, poll_to_key(mask));
> > > if (mask & EPOLLERR) {
> > > - if (poll->wqh)
> > > - remove_wait_queue(poll->wqh, &poll->wait);
> > > + vhost_poll_stop(poll);
> > > ret = -EINVAL;
> > > }
> > >
> > > --
> > > 2.7.4
> > >
_______________________________________________
Virtualization mailing list
Virtualization@lists.linux-foundation.org
https://lists.linuxfoundation.org/mailman/listinfo/virtualization
^ permalink raw reply
* Re: [PATCH net V2] vhost: correctly remove wait queue during poll failure
From: Michael S. Tsirkin @ 2018-03-27 13:59 UTC (permalink / raw)
To: Jason Wang; +Cc: kvm, virtualization, netdev, linux-kernel, Darren Kenny
In-Reply-To: <1522155052-13347-1-git-send-email-jasowang@redhat.com>
On Tue, Mar 27, 2018 at 08:50:52PM +0800, Jason Wang wrote:
> We tried to remove vq poll from wait queue, but do not check whether
> or not it was in a list before. This will lead double free. Fixing
> this by switching to use vhost_poll_stop() which zeros poll->wqh after
> removing poll from waitqueue to make sure it won't be freed twice.
>
> Cc: Darren Kenny <darren.kenny@oracle.com>
> Reported-by: syzbot+c0272972b01b872e604a@syzkaller.appspotmail.com
> Fixes: 2b8b328b61c79 ("vhost_net: handle polling errors when setting backend")
> Signed-off-by: Jason Wang <jasowang@redhat.com>
Acked-by: Michael S. Tsirkin <mst@redhat.com>
> ---
> Changes from V1:
> - tweak the commit log for to match the code
> ---
> drivers/vhost/vhost.c | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/drivers/vhost/vhost.c b/drivers/vhost/vhost.c
> index 1b3e8d2d..5d5a9d9 100644
> --- a/drivers/vhost/vhost.c
> +++ b/drivers/vhost/vhost.c
> @@ -212,8 +212,7 @@ int vhost_poll_start(struct vhost_poll *poll, struct file *file)
> if (mask)
> vhost_poll_wakeup(&poll->wait, 0, 0, poll_to_key(mask));
> if (mask & EPOLLERR) {
> - if (poll->wqh)
> - remove_wait_queue(poll->wqh, &poll->wait);
> + vhost_poll_stop(poll);
> ret = -EINVAL;
> }
>
> --
> 2.7.4
^ permalink raw reply
* Re: [PATCH v7 0/7] netdev: Eliminate duplicate barriers on weakly-ordered archs
From: David Miller @ 2018-03-27 14:00 UTC (permalink / raw)
To: okaya; +Cc: netdev, timur, sulrich, linux-arm-msm, linux-arm-kernel
In-Reply-To: <044d58fd-b6ea-6341-0cdf-d99c47fc241e@codeaurora.org>
From: Sinan Kaya <okaya@codeaurora.org>
Date: Tue, 27 Mar 2018 08:40:41 -0400
> Are you able to drop the applied ones so that I can post V7 or is it
> too late?
I cannot "drop" changes from my tree since my tree is used by thousands
of people and therefore immutable.
You must therefore send me relative fixes or reverts.
^ permalink raw reply
* Re: [PATCH v7 0/7] netdev: Eliminate duplicate barriers on weakly-ordered archs
From: Sinan Kaya @ 2018-03-27 14:02 UTC (permalink / raw)
To: David Miller; +Cc: netdev, timur, sulrich, linux-arm-msm, linux-arm-kernel
In-Reply-To: <20180327.100051.1676812408119987944.davem@davemloft.net>
On 3/27/2018 10:00 AM, David Miller wrote:
> From: Sinan Kaya <okaya@codeaurora.org>
> Date: Tue, 27 Mar 2018 08:40:41 -0400
>
>> Are you able to drop the applied ones so that I can post V7 or is it
>> too late?
>
> I cannot "drop" changes from my tree since my tree is used by thousands
> of people and therefore immutable.
>
> You must therefore send me relative fixes or reverts.
>
Thanks, I'll send fixes. Just wanted to see whether it got merged or if it was
sitting on a branch.
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [net-next PATCH 2/5] soc: ti: K2G: provide APIs to support driver probe deferral
From: Andrew Lunn @ 2018-03-27 14:03 UTC (permalink / raw)
To: Murali Karicheri
Cc: robh+dt, mark.rutland, ssantosh, malat, w-kwok2, devicetree,
linux-kernel, linux-arm-kernel, davem, netdev
In-Reply-To: <4aeeda56-c68b-cb64-1314-a669f721ff20@ti.com>
> Could you please elaborate? These knav dma and qmss drivers are
> introduced to support packet DMA hardware available in Keystone
> NetCP which couldn't be implemented using the DMA APIs available
> at the time this driver was introduced. Another reason was that
> the performance was really bad. We had an internal implementation
> based on DMA API before which couldn't be upstreamed at that time
> due to the reason that we were mis-using the API for this driver.
> So we introduced these knav_dma driver to support NetCP. We don't
> have any plan to re-write the driver at this time.
>
> If your question is about EPROBE_DEFER being returned from an
> existing knav_dma API and using the return code to achieve probe
> defer instead of introducing these APIs, I can take a look into
> that and respond. So please clarify.
Hi Murali
So if i understood you right, at the time these drivers were written,
the linux DMA API did not do what you wanted. You could hack something
together by using the API wrongly, but that could not be mainlined. So
rather than fixing the DMA API to make it work for this hardware, you
ignored it, and made up your own API? This API now has its own
problems, it does not correctly handle ordering? So you are hacking
your own API further.
Does the Linux DMA API correctly handle probing order issues? Has the
Linux DMA API evolved so that it now does do what is needed by your
hardware?
If this was an old hardware which is slowly going away, it would not
be an issue. But it seems like there are new variants of the hardware
being released. So maybe you should go back and re-write the DMA
driver, rather than paper over the cracks?
Andrew
^ permalink raw reply
* Re: Re: [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs
From: Lino Sanfilippo @ 2018-03-27 14:04 UTC (permalink / raw)
To: Sinan Kaya
Cc: Jeff Kirsher, Alexander Duyck, intel-wired-lan, Netdev,
Timur Tabi, sulrich, linux-arm-msm, linux-arm-kernel
In-Reply-To: <e3ada376-52a5-573b-33f1-9aa84af75f0d@codeaurora.org>
Hi,
> Double sorry now.
>
> I don't know if you have been following "RFC on writel and writel_relaxed" thread
> or not but there are some new developments about wmb() requirement.
Just out of interest: Where can this thread be found?
>
> Basically, wmb() should never be used before writel() as writel() seem to
> provide coherency and observability guarantee.
>
AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
IO-memory not RAM vs. IO-memory (this may be the case for some architectures
where the writel() implementation contains a wmb() but not for all).
For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used.
Is this not correct?
Regards,
Lino
^ permalink raw reply
* Re: [net-next 00/10][pull request] 40GbE Intel Wired LAN Driver Updates 2018-03-26
From: David Miller @ 2018-03-27 14:05 UTC (permalink / raw)
To: jeffrey.t.kirsher; +Cc: netdev, nhorman, sassmann, jogreene
In-Reply-To: <20180326214103.18218-1-jeffrey.t.kirsher@intel.com>
From: Jeff Kirsher <jeffrey.t.kirsher@intel.com>
Date: Mon, 26 Mar 2018 14:40:53 -0700
> This series contains updates to i40e only.
>
> Jake provides several patches which remove the need for cmpxchg64(),
> starting with moving I40E_FLAG_[UDP]_FILTER_SYNC from pf->flags to pf->state
> since they are modified during run time possibly when the RTNL lock is not
> held so they should be a state bits and not flags. Moved additional
> "flags" which should be state fields, into pf->state. Ensure we hold
> the RTNL lock for the entire sequence of preparing for reset and when
> resuming, which will protect the flags related to interrupt scheme under
> RTNL lock so that their modification is properly threaded. Finally,
> cleanup the use of cmpxchg64() since it is no longer needed. Cleaned up
> the holes in the feature flags created my moving some flags to the state
> field.
>
> Björn Töpel adds XDP_REDIRECT support as well as tweaking the page
> counting for XDP_REDIRECT so that it will function properly.
Pulled, thanks Jeff.
^ permalink raw reply
* Re: [PATCH v6 bpf-next 07/11] tracepoint: introduce kernel_tracepoint_find_by_name
From: Steven Rostedt @ 2018-03-27 14:07 UTC (permalink / raw)
To: Alexei Starovoitov
Cc: davem, daniel, torvalds, peterz, netdev, kernel-team, linux-api,
Mathieu Desnoyers
In-Reply-To: <20180327024706.2064725-8-ast@fb.com>
On Mon, 26 Mar 2018 19:47:02 -0700
Alexei Starovoitov <ast@fb.com> wrote:
> From: Alexei Starovoitov <ast@kernel.org>
>
> introduce kernel_tracepoint_find_by_name() helper to let bpf core
> find tracepoint by name and later attach bpf probe to a tracepoint
>
> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Thanks for doing this Alexei!
One nit below.
> ---
> include/linux/tracepoint.h | 6 ++++++
> kernel/tracepoint.c | 9 +++++++++
> 2 files changed, 15 insertions(+)
>
> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
> index c92f4adbc0d7..a00b84473211 100644
> --- a/include/linux/tracepoint.h
> +++ b/include/linux/tracepoint.h
> @@ -43,6 +43,12 @@ tracepoint_probe_unregister(struct tracepoint *tp, void *probe, void *data);
> extern void
> for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> void *priv);
> +#ifdef CONFIG_TRACEPOINTS
> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
> +#else
> +static inline struct tracepoint *
> +kernel_tracepoint_find_by_name(const char *name) { return NULL; }
> +#endif
>
> #ifdef CONFIG_MODULES
> struct tp_module {
> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
> index 671b13457387..e2a9a0391ae2 100644
> --- a/kernel/tracepoint.c
> +++ b/kernel/tracepoint.c
> @@ -528,6 +528,15 @@ void for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
> }
> EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
>
> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name)
> +{
> + struct tracepoint * const *tp = __start___tracepoints_ptrs;
> +
> + for (; tp < __stop___tracepoints_ptrs; tp++)
> + if (!strcmp((*tp)->name, name))
> + return *tp;
Usually for cases like this, we prefer to add brackets for the for
block, as it's not a single line below it.
for (; tp < __stop__tracepoints_ptrs; tp++) {
if (!strcmp((*tp)->name, name))
return *tp;
}
-- Steve
> + return NULL;
> +}
> #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
>
> /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
^ permalink raw reply
* Re: [PATCH v6 bpf-next 07/11] tracepoint: introduce kernel_tracepoint_find_by_name
From: Mathieu Desnoyers @ 2018-03-27 14:18 UTC (permalink / raw)
To: rostedt
Cc: Alexei Starovoitov, David S. Miller, Daniel Borkmann,
Linus Torvalds, Peter Zijlstra, netdev, kernel-team, linux-api
In-Reply-To: <20180327100711.0ec7aafb@gandalf.local.home>
----- On Mar 27, 2018, at 10:07 AM, rostedt rostedt@goodmis.org wrote:
> On Mon, 26 Mar 2018 19:47:02 -0700
> Alexei Starovoitov <ast@fb.com> wrote:
>
>> From: Alexei Starovoitov <ast@kernel.org>
>>
>> introduce kernel_tracepoint_find_by_name() helper to let bpf core
>> find tracepoint by name and later attach bpf probe to a tracepoint
>>
>> Signed-off-by: Alexei Starovoitov <ast@kernel.org>
>
> Reviewed-by: Steven Rostedt (VMware) <rostedt@goodmis.org>
Steven showed preference for tracepoint_kernel_find_by_name() at some
point (starting with a tracepoint_ prefix). I'm find with either of
the names.
Reviewed-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
Thanks,
Mathieu
>
> Thanks for doing this Alexei!
>
> One nit below.
>
>
>> ---
>> include/linux/tracepoint.h | 6 ++++++
>> kernel/tracepoint.c | 9 +++++++++
>> 2 files changed, 15 insertions(+)
>>
>> diff --git a/include/linux/tracepoint.h b/include/linux/tracepoint.h
>> index c92f4adbc0d7..a00b84473211 100644
>> --- a/include/linux/tracepoint.h
>> +++ b/include/linux/tracepoint.h
>> @@ -43,6 +43,12 @@ tracepoint_probe_unregister(struct tracepoint *tp, void
>> *probe, void *data);
>> extern void
>> for_each_kernel_tracepoint(void (*fct)(struct tracepoint *tp, void *priv),
>> void *priv);
>> +#ifdef CONFIG_TRACEPOINTS
>> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name);
>> +#else
>> +static inline struct tracepoint *
>> +kernel_tracepoint_find_by_name(const char *name) { return NULL; }
>> +#endif
>>
>> #ifdef CONFIG_MODULES
>> struct tp_module {
>> diff --git a/kernel/tracepoint.c b/kernel/tracepoint.c
>> index 671b13457387..e2a9a0391ae2 100644
>> --- a/kernel/tracepoint.c
>> +++ b/kernel/tracepoint.c
>> @@ -528,6 +528,15 @@ void for_each_kernel_tracepoint(void (*fct)(struct
>> tracepoint *tp, void *priv),
>> }
>> EXPORT_SYMBOL_GPL(for_each_kernel_tracepoint);
>>
>> +struct tracepoint *kernel_tracepoint_find_by_name(const char *name)
>> +{
>> + struct tracepoint * const *tp = __start___tracepoints_ptrs;
>> +
>> + for (; tp < __stop___tracepoints_ptrs; tp++)
>> + if (!strcmp((*tp)->name, name))
>> + return *tp;
>
>
> Usually for cases like this, we prefer to add brackets for the for
> block, as it's not a single line below it.
>
> for (; tp < __stop__tracepoints_ptrs; tp++) {
> if (!strcmp((*tp)->name, name))
> return *tp;
> }
>
> -- Steve
>
>
>
>
>> + return NULL;
>> +}
>> #ifdef CONFIG_HAVE_SYSCALL_TRACEPOINTS
>>
> > /* NB: reg/unreg are called while guarded with the tracepoints_mutex */
--
Mathieu Desnoyers
EfficiOS Inc.
http://www.efficios.com
^ permalink raw reply
* [PATCH][next] ixgbevf: ensure xdp_ring resources are free'd on error exit
From: Colin King @ 2018-03-27 14:21 UTC (permalink / raw)
To: Jeff Kirsher, intel-wired-lan, netdev; +Cc: kernel-janitors, linux-kernel
From: Colin Ian King <colin.king@canonical.com>
The current error handling for failed resource setup for xdp_ring
data is a break out of the loop and returning 0 indicated everying
was OK, when in fact it is not. Fix this by exiting via the
error exit label err_setup_tx that will clean up the resources
correctly and return and error status.
Detected by CoverityScan, CID#1466879 ("Logically dead code")
Fixes: 21092e9ce8b1 ("ixgbevf: Add support for XDP_TX action")
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
index 3d9033f26eff..e3d04f226d57 100644
--- a/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
+++ b/drivers/net/ethernet/intel/ixgbevf/ixgbevf_main.c
@@ -3420,7 +3420,7 @@ static int ixgbevf_setup_all_tx_resources(struct ixgbevf_adapter *adapter)
if (!err)
continue;
hw_dbg(&adapter->hw, "Allocation for XDP Queue %u failed\n", j);
- break;
+ goto err_setup_tx;
}
return 0;
--
2.15.1
^ permalink raw reply related
* Re: [PATCH net] sctp: remove unnecessary asoc in sctp_has_association
From: David Miller @ 2018-03-27 14:22 UTC (permalink / raw)
To: lucien.xin; +Cc: netdev, linux-sctp, marcelo.leitner, nhorman
In-Reply-To: <955d9f51fafeb670cb4468e6253f4e54cd3c90f0.1522054500.git.lucien.xin@gmail.com>
From: Xin Long <lucien.xin@gmail.com>
Date: Mon, 26 Mar 2018 16:55:00 +0800
> After Commit dae399d7fdee ("sctp: hold transport instead of assoc
> when lookup assoc in rx path"), it put transport instead of asoc
> in sctp_has_association. Variable 'asoc' is not used any more.
>
> So this patch is to remove it, while at it, it also changes the
> return type of sctp_has_association to bool, and does the same
> for it's caller sctp_endpoint_is_peeled_off.
>
> Signed-off-by: Xin Long <lucien.xin@gmail.com>
Since this is just a cleanup and a simplification, and doesn't fix
any bugs, I'm applying this to net-next.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 0/7] netdev: intel: Eliminate duplicate barriers on weakly-ordered archs
From: Sinan Kaya @ 2018-03-27 14:23 UTC (permalink / raw)
To: Lino Sanfilippo
Cc: Jeff Kirsher, Alexander Duyck, intel-wired-lan, Netdev,
Timur Tabi, sulrich, linux-arm-msm, linux-arm-kernel
In-Reply-To: <trinity-6d247cd1-94e3-4a58-a762-77724e895644-1522159460624@3c-app-gmx-bs61>
On 3/27/2018 10:04 AM, Lino Sanfilippo wrote:
> Hi,
>
>> Double sorry now.
>>
>> I don't know if you have been following "RFC on writel and writel_relaxed" thread
>> or not but there are some new developments about wmb() requirement.
>
> Just out of interest: Where can this thread be found?
https://www.spinics.net/lists/linux-rdma/msg62570.html
https://patchwork.kernel.org/patch/10309913/
>
>>
>> Basically, wmb() should never be used before writel() as writel() seem to
>> provide coherency and observability guarantee.
>>
>
> AFAIU memory-barriers.txt writel() only guarantees correct order of accesses to
> IO-memory not RAM vs. IO-memory (this may be the case for some architectures
> where the writel() implementation contains a wmb() but not for all).
> For the RAM vs. IO-memory case at least a a wmb()/rmb() has to be used.
> Is this not correct?
We are being told that if you use writel(), then you don't need a wmb() on
all architectures.
Jason is seeking behavior clarification for write combined buffers.
>
> Regards,
> Lino
>
--
Sinan Kaya
Qualcomm Datacenter Technologies, Inc. as an affiliate of Qualcomm Technologies, Inc.
Qualcomm Technologies, Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project.
^ permalink raw reply
* Re: [PATCH v3 net 1/5] tcp: feed correct number of pkts acked to cc modules also in recovery
From: Ilpo Järvinen @ 2018-03-27 14:23 UTC (permalink / raw)
To: Yuchung Cheng; +Cc: netdev, Neal Cardwell, Eric Dumazet, Sergei Shtylyov
In-Reply-To: <CAK6E8=ed3Lt4seZNv+veaVT-U893FVHqWGWhzwfEXyeb-yAkhA@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 7955 bytes --]
On Mon, 26 Mar 2018, Yuchung Cheng wrote:
> On Tue, Mar 13, 2018 at 3:25 AM, Ilpo Järvinen
> <ilpo.jarvinen@helsinki.fi> wrote:
> >
> > A miscalculation for the number of acknowledged packets occurs during
> > RTO recovery whenever SACK is not enabled and a cumulative ACK covers
> > any non-retransmitted skbs. The reason is that pkts_acked value
> > calculated in tcp_clean_rtx_queue is not correct for slow start after
> > RTO as it may include segments that were not lost and therefore did
> > not need retransmissions in the slow start following the RTO. Then
> > tcp_slow_start will add the excess into cwnd bloating it and
> > triggering a burst.
> >
> > Instead, we want to pass only the number of retransmitted segments
> > that were covered by the cumulative ACK (and potentially newly sent
> > data segments too if the cumulative ACK covers that far).
> >
> > Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@helsinki.fi>
> > ---
> > net/ipv4/tcp_input.c | 16 +++++++++++++++-
> > 1 file changed, 15 insertions(+), 1 deletion(-)
> >
> > diff --git a/net/ipv4/tcp_input.c b/net/ipv4/tcp_input.c
> > index 9a1b3c1..4a26c09 100644
> > --- a/net/ipv4/tcp_input.c
> > +++ b/net/ipv4/tcp_input.c
> > @@ -3027,6 +3027,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
> > long seq_rtt_us = -1L;
> > long ca_rtt_us = -1L;
> > u32 pkts_acked = 0;
> > + u32 rexmit_acked = 0;
> > + u32 newdata_acked = 0;
> > u32 last_in_flight = 0;
> > bool rtt_update;
> > int flag = 0;
> > @@ -3056,8 +3058,10 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
> > }
> >
> > if (unlikely(sacked & TCPCB_RETRANS)) {
> > - if (sacked & TCPCB_SACKED_RETRANS)
> > + if (sacked & TCPCB_SACKED_RETRANS) {
> > tp->retrans_out -= acked_pcount;
> > + rexmit_acked += acked_pcount;
> > + }
> > flag |= FLAG_RETRANS_DATA_ACKED;
> > } else if (!(sacked & TCPCB_SACKED_ACKED)) {
> > last_ackt = skb->skb_mstamp;
> > @@ -3070,6 +3074,8 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
> > reord = start_seq;
> > if (!after(scb->end_seq, tp->high_seq))
> > flag |= FLAG_ORIG_SACK_ACKED;
> > + else
> > + newdata_acked += acked_pcount;
> > }
> >
> > if (sacked & TCPCB_SACKED_ACKED) {
> > @@ -3151,6 +3157,14 @@ static int tcp_clean_rtx_queue(struct sock *sk, u32 prior_fack,
> > }
> >
> > if (tcp_is_reno(tp)) {
> > + /* Due to discontinuity on RTO in the artificial
> > + * sacked_out calculations, TCP must restrict
> > + * pkts_acked without SACK to rexmits and new data
> > + * segments
> > + */
> > + if (icsk->icsk_ca_state == TCP_CA_Loss)
> > + pkts_acked = rexmit_acked + newdata_acked;
> > +
> My understanding is there are two problems
>
> 1) your fix: the reordering logic in tcp-remove_reno_sacks requires
> precise cumulatively acked count, not newly acked count?
While I'm not entirely sure if you intented to say that my fix is broken
or not, I thought this very difference alot while making the fix and I
believe that this fix is needed because of the discontinuity at RTO
(sacked_out is cleared as we set L-bits + lost_out). This is an artifact
in the imitation of sacked_out for non-SACK but at RTO we can't keep that
in sync because we set L-bits (and have no S-bits to guide us). Thus, we
cannot anymore "use" those skbs with only L-bit for the reno_sacks logic.
In tcp_remove_reno_sacks acked - sacked_out is being used to calculate
tp->delivered, using plain cumulative acked causes congestion control
breakage later as call to tcp_cong_control will directly use the
difference in tp->delivered.
This boils down the exact definition of tp->delivered (the one given in
the header is not detailed enough). I guess you might have better idea
what it exactly is since one of you has added it? There are subtle things
in the defination that can make it entirely unsuitable for cc decisions.
Should those segments that we (possibly) already counted into
tp->delivered during (potentially preceeding) CA_Recovery be added to it
for _second time_ or not? This fix avoids such double counting (the
price is that we might underestimate). For SACK+ACK losses, the similar
question is: Should those segments that we missed counting into
tp->delivered during preceeding CA_Recovery (due to losing enough SACKs)
be added into tp->delivered now during RTO recovery or not (I'm not
proposing we fix this unless we want to fix the both issues at the same
time here as its impact with SACK is not that significant)? Is
tp->delivered supposed to under- or overestimate (in the cases we're not
sure what/when something happened)? ...If it's overestimating under any
circumstances (for the current ACK), we cannot base our cc decision on it.
tcp_check_reno_reordering might like to have the cumulatively acked count
but due to the forementioned discontinuity we anyway cannot accurately
provide in CA_Loss what tcp_limit_reno_sacked expects (and the
other CA states are unaffected by this fix). While we could call
tcp_check_reno_reordering directly from tcp_clean_rtx_queue, it wouldn't
remove the fundamental discontinuity problem that we have for what
tcp_limit_reno_sacked assumes about sacked_out. It might actually be so
that tcp_limit_reno_sacked is just never going to work after we zero
tp->sacked_out (this would actually be the problem #3) or at least ends
up underestimating the reordering degree by the amount we cleared from
sacked_out at RTO?
> 2) current code: pkts_acked can substantially over-estimate the newly
> delivered pkts in both SACK and non-SACK cases. For example, let's say
> 99/100 packets are already sacked, and the next ACK acks 100 pkts.
> pkts_acked == 100 but really only one packet is delivered. It's wrong
> to inform congestion control that 100 packets have just delivered.
> AFAICT, the CCs that have pkts_acked callbacks all treat pkts_acked as
> the newly delivered packets.
>
> A better fix for both SACK and non-SACK, seems to be moving
> ca_ops->pkts_acked into tcp_cong_control, where the "acked_sacked" is
> calibrated? this is what BBR is currently doing to avoid these pitfalls.
Unfortunately that would not fix the problematic calculation of
tp->delivered.
But you might be right that there's a problem #2 (it's hard to notice
for real though as most of the cc modules don't seem to use it for
anything). However, this is for pkts_acked so are you sure what the
cc modules exactly expect: the shrinkage in # of outstanding packets or
the number of newly delivered packets?
Looking (only) quickly into the modules (that use it other than in
CA_Open):
- Somewhat suspicious delayed ACK detection logic in cdg
- HTCP might want shrinkage in # of outstanding packets (but I'm not
entirely sure) for throughput measurement
- I guess TCP illinois is broken (assumes it's newly delivered packets)
but it would not need to use it at all as it just proxies the value in
a local variable into tcp_illinois_cong_avoid that has the correct acked
readily available.
There are separate cong_avoid and cong_control callbacks that are more
obviously oriented for doing cc where as I'd think pkts_acked callback
seems more oriented to for RTT-sample related operations. Therefore, I'm
not entirely sure I this is what is wanted for pkts_acked, especially
given the HTCP example.
--
i.
^ permalink raw reply
* Re: [RFC PATCH v2] net: phy: Added device tree binding for dev-addr and dev-addr code check-up
From: Andrew Lunn @ 2018-03-27 14:24 UTC (permalink / raw)
To: Vicenţiu Galanopulo
Cc: Rob Herring, netdev@vger.kernel.org, linux-kernel@vger.kernel.org,
mark.rutland@arm.com, davem@davemloft.net, marcel@holtmann.org,
devicetree@vger.kernel.org, Madalin-cristian Bucur,
Alexandru Marginean
In-Reply-To: <HE1PR0402MB3578B3D33160EBE59C2FD7D7EEAC0@HE1PR0402MB3578.eurprd04.prod.outlook.com>
> > This is a 2nd MDIO address, right? Can't you just append this to reg property?
Hi Rob
It is a sub address.
There are two different MDIO addressing schemes. Clause 22 allowed for
32 different addresses on an MDIO bus. Clause 45 extended that. You
have the existing 32 addresses for a package. However, within a
package, you can have 32 devices.
You are supposed to be able to look are registers inside the package,
and it will tell you which devices in the packages are in use. You can
then look at those devices and figure out what they are using ID
registers.
However some vendors get this wrong, they don't fill in the devices in
package information. So the generic probe code never finds them. We
need to pass it a hint, go looking at this specific device in the
package.
You can mix Clause 22 and Clause 45 on the same bus. Does DT allow two
different reg formats to be used at once? Can we have some reg
properties with a single value, and some with two values? I thought
#address-cells = <1> means there should be a single address in reg.
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