* Re: [PATCH] Revert "net: stmmac: Do not keep rearming the coalesce timer in stmmac_xmit"
From: Jose Abreu @ 2018-08-28 8:12 UTC (permalink / raw)
To: Jerome Brunet, Giuseppe Cavallaro, Alexandre Torgue, Jose Abreu,
netdev
Cc: linux-kernel, linux-amlogic, Joao Pinto, Vitor Soares,
Corentin Labbe
In-Reply-To: <20180824090440.13411-1-jbrunet@baylibre.com>
Hi Jerome,
On 24-08-2018 10:04, Jerome Brunet wrote:
> This reverts commit 4ae0169fd1b3c792b66be58995b7e6b629919ecf.
>
> This change in the handling of the coalesce timer is causing regression on
> (at least) amlogic platforms.
>
> Network will break down very quickly (a few seconds) after starting
> a download. This can easily be reproduced using iperf3 for example.
>
> The problem has been reported on the S805, S905, S912 and A113 SoCs
> (Realtek and Micrel PHYs) and it is likely impacting all Amlogics
> platforms using Gbit ethernet
>
> No problem was seen with the platform using 10/100 only PHYs (GXL internal)
>
> Reverting change brings things back to normal and allows to use network
> again until we better understand the problem with the coalesce timer.
>
>
Apologies for the delayed answer but I was in FTO.
I'm not sure what can be causing this but I have some questions
for you:
- What do you mean by "network will break down"? Do you see
queue timeout?
- What do you see in ethtool/ifconfig stats? Can you send me
the stats before and after network break?
- Is your setup multi-queue/channel?
- Can you point me to the DT bindings of your setup?
Thanks and Best Regards,
Jose Miguel Abreu
^ permalink raw reply
* Re: [PATCH 00/15] soc: octeontx2: Add RVU admin function driver
From: Arnd Bergmann @ 2018-08-28 12:23 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Linux Kernel Mailing List, Olof Johansson, Linux ARM, linux-soc,
sgoutham, Networking, David Miller
In-Reply-To: <1535453838-12154-1-git-send-email-sunil.kovvuri@gmail.com>
On Tue, Aug 28, 2018 at 12:57 PM <sunil.kovvuri@gmail.com> wrote:
>
> From: Sunil Goutham <sgoutham@marvell.com>
>
> Resource virtualization unit (RVU) on Marvell's OcteonTX2 SOC supports
> multiple PCIe SRIOV physical functions (PFs) and virtual functions (VFs).
> PF0 is called administrative / admin function (AF) and has privilege access
> to registers to provision different RVU functional blocks to each of
> PF/VF.
>
> This admin function (AF) driver acts as a configuration / administrative
> software which provisions functional blocks to a PF/VF on demand for them
> to work as one of the following
> - A basic network controller (i.e NIC).
> - NIC with packet filtering, shaping and scheduling capabilities.
> - A crypto device.
> - A combination of above etc.
>
> PF/VFs communicate with admin function via a shared memory region.
> This patch series adds logic for the following
> - RVU AF driver with functional blocks provisioning support
> - Mailbox infrastructure for communication between AF and PFs.
> - CGX driver which provides information about physcial network
> interfaces which AF processes and forwards required info to
> PF/VF drivers.
>
> This is the first set of patches out of 70 odd patches.
>
> Note: This driver neither receives any data nor processes it i.e no I/O,
> just does the hardware configuration.
Hi Sunil,
Thanks for posting this first series, I'm glad we're seeing support for this
chip family making some progress.
My feeling overall is that we need a review from the network driver
folks more than the arm-soc team etc, and that maybe the driver
as a whole should go into drivers/net/ethernet.
We support some couple of similar hardware already that has
both support for virtual functions and for crypto offload, including
the Chelsio cxgb4, Mellanox mlx5, NXP DPAA and probably others,
and we need to ensure that the exposed interfaces are all
compatible, and that you use the correct subsystems and
in-kernel abstractions for thing that are common.
Arnd
^ permalink raw reply
* Re: [PATCH] mac80211: fix to follow standard
From: Kalle Valo @ 2018-08-28 12:44 UTC (permalink / raw)
To: Yuan-Chi Pang; +Cc: johannes, davem, linux-wireless, netdev, linux-kernel
In-Reply-To: <1535423074-130862-1-git-send-email-fu3mo6goo@gmail.com>
Yuan-Chi Pang <fu3mo6goo@gmail.com> writes:
> IEEE 802.11-2016 14.10.8.3 HWMP sequence numbering says:
> If it is a target mesh STA, it shall update its own HWMP SN to
> maximum (current HWMP SN, target HWMP SN in the PREQ element) + 1
> immediately before it generates a PREP element in response to a
> PREQ element.
>
> Signed-off-by: Yuan-Chi Pang <fu3mo6goo@gmail.com>
The patch title is quite generic, you could make it a bit more
informative and unique. For example:
mac80211: fix HWMP sequence numbering to follow standard
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH net-next] Implement a rtnetlink device which simulates wifi.
From: Johannes Berg @ 2018-08-28 12:45 UTC (permalink / raw)
To: Cody Schuffelen, Kalle Valo, David S. Miller, linux-kernel,
linux-wireless, netdev, kernel-team
In-Reply-To: <5b5b8e09.1c69fb81.c693c.0acd@mx.google.com>
Hi Cody,
Sorry for the delay in reviewing this.
I think in general there may be some value in this.
I think you'd have a far better experience (and some more real wifi
testing) by adding the ability to route the hwsim virtual air/medium of
the inside of the VM to the outside of the VM, and then you could run
APs there and bridge to ethernet etc. That way, you could have far more
realistic WiFi with multiple APs etc. with not all that much effort.
Regarding the patch itself:
On Tue, 2018-07-24 at 17:10 -0700, Cody Schuffelen wrote:
> The device added here is used through "ip link add ... type virt_wifi"
Can you have a more elaborate example that includes how to specify the
ethernet link to use?
> +static struct ieee80211_channel channel = {
> + .band = NL80211_BAND_5GHZ,
You only have a single 5 GHz channel? That's pretty atypical - there are
no real devices that behave this way. I wouldn't be surprised if some
userspace assumes you have at least one 2.4 GHz channel, so I'd argue
you should have a more regular setup here.
> +static struct ieee80211_rate bitrate = {
> + .flags = IEEE80211_RATE_SHORT_PREAMBLE, /* ieee80211_rate_flags */
> + .bitrate = 1000,
err, well, you can't really have 10Mbps as a rate ... it doesn't exist.
Also, short preamble is irrelevant on 5 GHz.
Again, you should have a more regular setup here.
> +};
> +
> +static struct ieee80211_supported_band band_5ghz = {
> + .channels = &channel,
> + .bitrates = &bitrate,
> + .band = NL80211_BAND_5GHZ,
> + .n_channels = 1,
> + .n_bitrates = 1,
> +};
And probably best to support the normal 8 bitrates supported on 5GHz at
least?
Or perhaps put the whole thing on 2.4GHz since chips actually exist that
only have 2.4 (vs. none that only have 5), and support some HT/VHT even
to make it look like it's not out of the last millenium? :)
> +static struct cfg80211_inform_bss mock_inform_bss = {
> + /* ieee80211_channel* */ .chan = &channel,
> + /* nl80211_bss_scan_width */ .scan_width = NL80211_BSS_CHAN_WIDTH_20,
> + /* s32 */ .signal = 99,
That signal level is questionable ... better limit to something actually
used in practice like -60 dBm?
> +static u8 fake_router_bssid[] = {4, 4, 4, 4, 4, 4};
Well, use a locally assigned address at least if you're going to fake
something?
> + for (i = 0; i < request->n_ssids; i++) {
> + strncpy(request_ssid_copy, request->ssids[i].ssid,
> + request->ssids[i].ssid_len);
> + request_ssid_copy[request->ssids[i].ssid_len] = 0;
> + wiphy_debug(wiphy, "scan: ssid: %s\n",
> + request_ssid_copy);
SSIDs can very well contain NUL bytes, so this is not a valid way to
print them.
Why would you even want to print the request anyway though, if your
stated goal is to make it look to userspace like a wifi link, vs.
debugging?
Plus you can always run "iw event" to see it.
> + }
> + }
> +
> + priv->scan_request = request;
> + schedule_delayed_work(&priv->scan_result, HZ / 100);
That's way too fast, so for any realism you should probably wait 2-3
seconds. Well, you only have one channel, so perhaps only 100-200ms? But
10ms is not realistic.
> + return 0;
> +}
> +
> +static void virt_wifi_scan_result(struct work_struct *work)
> +{
> + struct virt_wifi_priv *priv =
> + container_of(work, struct virt_wifi_priv,
> + scan_result.work);
> + struct wiphy *wiphy = priv_to_wiphy(priv);
> + char ssid[] = "__VirtWifi";
> + struct cfg80211_bss *informed_bss;
> +
> + mock_inform_bss.boottime_ns = ktime_get_boot_ns();
Updating the static variable seems a bit weird, this could be concurrent
for different instances, afaict?
> +
> +static struct ieee80211_mgmt auth_mgmt_frame = {
> + .frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT
> + | IEEE80211_STYPE_AUTH),
more idiomatic kernel coding style would put the | at the end of the
first line
> + .duration = cpu_to_le16(1), /* ??? */
that's not quite a realistic duration, but I guess nobody cares
> + .u = {
> + .auth = {
> + .auth_alg = WLAN_AUTH_OPEN,
> + /* auth request has 1, auth response has 2 */
> + .auth_transaction = cpu_to_le16(2),
> + },
> + },
> +};
> +
> +static int virt_wifi_auth(struct wiphy *wiphy, struct net_device *dev,
> + struct cfg80211_auth_request *req)
> +{
> + wiphy_debug(wiphy, "auth\n");
> + memcpy(auth_mgmt_frame.da, dev->dev_addr, dev->addr_len);
> + memcpy(auth_mgmt_frame.sa, fake_router_bssid,
> + sizeof(fake_router_bssid));
> + memcpy(auth_mgmt_frame.bssid, fake_router_bssid,
> + sizeof(fake_router_bssid));
Well, I guess at the very least you should check you're connecting to
the right AP? Or maybe not, since you only fake one to exist in the
first place.
Same problem with globals being modified though.
> + /* Must call cfg80211_rx_mlme_mgmt to notify about the response to this.
> + * This must hold the mutex for the wedev while calling the function.
typo - wdev
> + * Luckily the nl80211 code invoking this already holds that mutex.
> + */
> + cfg80211_rx_mlme_mgmt(dev, (const u8 *)&auth_mgmt_frame,
> + sizeof(auth_mgmt_frame));
Some delay might be appropriate.
> + return 0;
> +}
> +
> +static struct ieee80211_mgmt assoc_mgmt_frame = {
> + .frame_control = cpu_to_le16(IEEE80211_FTYPE_MGMT
> + | IEEE80211_STYPE_ASSOC_RESP),
> + .duration = cpu_to_le16(1), /* ??? */
see above
> +static int virt_wifi_assoc(struct wiphy *wiphy, struct net_device *dev,
> + struct cfg80211_assoc_request *req)
> +{
> + wiphy_debug(wiphy, "assoc\n");
> + memcpy(assoc_mgmt_frame.da, dev->dev_addr, dev->addr_len);
see above
> + memcpy(assoc_mgmt_frame.sa, fake_router_bssid,
> + sizeof(fake_router_bssid));
> + memcpy(assoc_mgmt_frame.bssid, fake_router_bssid,
> + sizeof(fake_router_bssid));
> + /* Must call cfg80211_rx_assoc_resp to notify about the response to
> + * this. This must hold the mutex for the wedev while calling the
see above
> + * function. Luckily the nl80211 code invoking this already holds that
> + * mutex.
> + */
> + cfg80211_rx_assoc_resp(dev, req->bss, (const u8 *)&assoc_mgmt_frame,
> + sizeof(assoc_mgmt_frame), -1);
see above
(and this repeats for deauth and disassoc too)
> +static int virt_wifi_get_station(struct wiphy *wiphy, struct net_device *dev,
> + const u8 *mac, struct station_info *sinfo)
> +{
> + wiphy_debug(wiphy, "get_station\n");
> + /* Only the values used by netlink_utils.cpp. */
What's netlink_utils.cpp? :-)
More seriously though, if you're proposing a general addition, the
hidden Android reference isn't all that helpful.
> + sinfo->filled = BIT(NL80211_STA_INFO_TX_PACKETS) |
> + BIT(NL80211_STA_INFO_TX_FAILED) | BIT(NL80211_STA_INFO_SIGNAL) |
> + BIT(NL80211_STA_INFO_TX_BITRATE);
> + sinfo->tx_packets = 1;
> + sinfo->tx_failed = 0;
> + sinfo->signal = -1; /* -1 is the maximum signal strength, somehow. */
Yeah, well, -1 is like taking a jet engine next to your ear ... not
going to happen. Go with something reasonable instead, maybe -60?
> + sinfo->txrate = (struct rate_info) {
> + .legacy = 10000, /* units are 100kbit/s */
> + };
That's not realistic.
> + return 0;
> +}
> +
> +static const struct cfg80211_ops virt_wifi_cfg80211_ops = {
> + .scan = virt_wifi_scan,
> +
> + .auth = virt_wifi_auth,
> + .assoc = virt_wifi_assoc,
> + .deauth = virt_wifi_deauth,
> + .disassoc = virt_wifi_disassoc,
> +
> + .get_station = virt_wifi_get_station,
That seems a little *too* minimal, and too much at the same time...
Practially no drivers, apart from mac80211 ones, implement auth/assoc -
in particular not most chips used in Android. So connect/disconnect
instead of auth/assoc/deauth/disassoc might be more realistic.
On the other hand, if you have get_station you should probably have
dump_station so people can use tools other than Android.
> + /* 100 SSIDs should be enough for anyone! */
> + wiphy->max_scan_ssids = 101;
Well, typical implementations limit this to 4, or 20, so this is
certainly excessive.
> + /* Don't worry about frequency regulations. */
> + wiphy->regulatory_flags = REGULATORY_WIPHY_SELF_MANAGED;
> + wiphy->interface_modes = BIT(NL80211_IFTYPE_STATION) |
> + BIT(NL80211_IFTYPE_AP) |
> + BIT(NL80211_IFTYPE_P2P_CLIENT) |
> + BIT(NL80211_IFTYPE_P2P_GO) |
> + BIT(NL80211_IFTYPE_ADHOC) |
> + BIT(NL80211_IFTYPE_MESH_POINT) |
> + BIT(NL80211_IFTYPE_MONITOR);
You obviously only support NL80211_IFTYPE_STATION - the other modes
require many more ops to be implemented.
> + wiphy->flags |= WIPHY_FLAG_SUPPORTS_TDLS |
> + WIPHY_FLAG_HAS_REMAIN_ON_CHANNEL |
> + WIPHY_FLAG_AP_UAPSD |
> + WIPHY_FLAG_HAS_CHANNEL_SWITCH;
Nope, has none of these.
> + wiphy->features |= NL80211_FEATURE_ACTIVE_MONITOR |
> + NL80211_FEATURE_AP_MODE_CHAN_WIDTH_CHANGE |
> + NL80211_FEATURE_STATIC_SMPS |
> + NL80211_FEATURE_DYNAMIC_SMPS |
> + NL80211_FEATURE_AP_SCAN |
> + NL80211_FEATURE_SCAN_RANDOM_MAC_ADDR;
None of these either.
> +static netdev_tx_t virt_wifi_start_xmit(struct sk_buff *skb,
> + struct net_device *dev)
> +{
> + struct virt_wifi_netdev_priv *priv = netdev_priv(dev);
> +
> + skb->dev = priv->lowerdev;
> + return dev_queue_xmit(skb);
This really should only work while connected on "wifi".
> +static const struct net_device_ops wifi_vlan_ops = {
vlan?
> +/* Called under rcu_read_lock() from netif_receive_skb */
> +static rx_handler_result_t virt_wifi_rx_handler(struct sk_buff **pskb)
> +{
> + struct sk_buff *skb = *pskb;
> + struct virt_wifi_netdev_priv *priv =
> + rcu_dereference(skb->dev->rx_handler_data);
> +
> + /* macvlan uses GFP_ATOMIC here. */
so?
> + skb = skb_share_check(skb, GFP_ATOMIC);
> + if (!skb) {
> + dev_err(&priv->upperdev->dev, "can't skb_share_check\n");
> + return RX_HANDLER_CONSUMED;
> + }
> +
> + *pskb = skb;
> + skb->dev = priv->upperdev;
> + skb->pkt_type = PACKET_HOST;
> + return RX_HANDLER_ANOTHER;
> +}
I guess this should also only do something while connected.
johannes
^ permalink raw reply
* Re: [PATCH 04/15] soc: octeontx2: Add mailbox support infra
From: Arnd Bergmann @ 2018-08-28 12:52 UTC (permalink / raw)
To: Sunil Kovvuri
Cc: Linux Kernel Mailing List, Olof Johansson, Linux ARM, linux-soc,
amakarov, sgoutham, lbartosik, Networking, David Miller
In-Reply-To: <CA+sq2CeyQ8r7bj3vK=cfriif=HhUu36ZUPWqxXm1e0DktHG+Fg@mail.gmail.com>
On Tue, Aug 28, 2018 at 2:48 PM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
>
> On Tue, Aug 28, 2018 at 5:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
> >
> > On Tue, Aug 28, 2018 at 12:57 PM <sunil.kovvuri@gmail.com> wrote:
> > >
> > > From: Aleksey Makarov <amakarov@marvell.com>
> > >
> > > This patch adds mailbox support infrastructure APIs.
> > > Each RVU device has a dedicated 64KB mailbox region
> > > shared with it's peer for communication. RVU AF has
> > > a separate mailbox region shared with each of RVU PFs
> > > and a RVU PF has a separate region shared with each of
> > > it's VF.
> > >
> > > These set of APIs are used by this driver (RVU AF) and
> > > other RVU PF/VF drivers eg netdev, crypto e.t.c.
> > >
> > > Signed-off-by: Aleksey Makarov <amakarov@marvell.com>
> > > Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> > > Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> >
> > Why does this driver not use the drivers/mailbox/ infrastructure?
> >
> This is a common administrative software driver which will be handling requests
> from kernel drivers and as well as drivers in userspace applications.
> We had to keep mailbox communication infrastructure same across all usages.
Can you explain more about the usage of userspace applications
and what interface you plan to use into the kernel?
Do you things like AF_XDP and virtual machines, or something else?
Arnd
^ permalink raw reply
* Re: [PATCH] mac80211: fix to follow standard
From: Sergei Shtylyov @ 2018-08-28 9:23 UTC (permalink / raw)
To: Yuan-Chi Pang, johannes; +Cc: davem, linux-wireless, netdev, linux-kernel
In-Reply-To: <1535423074-130862-1-git-send-email-fu3mo6goo@gmail.com>
Hello!
On 8/28/2018 5:24 AM, Yuan-Chi Pang wrote:
> IEEE 802.11-2016 14.10.8.3 HWMP sequence numbering says:
> If it is a target mesh STA, it shall update its own HWMP SN to
> maximum (current HWMP SN, target HWMP SN in the PREQ element) + 1
> immediately before it generates a PREP element in response to a
> PREQ element.
>
> Signed-off-by: Yuan-Chi Pang <fu3mo6goo@gmail.com>
> ---
> net/mac80211/mesh_hwmp.c | 5 +++++
> 1 file changed, 5 insertions(+)
>
> diff --git a/net/mac80211/mesh_hwmp.c b/net/mac80211/mesh_hwmp.c
> index 35ad398..6c21a26 100644
> --- a/net/mac80211/mesh_hwmp.c
> +++ b/net/mac80211/mesh_hwmp.c
> @@ -572,6 +572,11 @@ static void hwmp_preq_frame_process(struct ieee80211_sub_if_data *sdata,
> forward = false;
> reply = true;
> target_metric = 0;
> +
> + if (SN_GT(target_sn, ifmsh->sn)) {
> + ifmsh->sn = target_sn;
> + }
No need for {} enclosing a single statement.
[...]
MBR, Sergei
^ permalink raw reply
* Re: [PATCH 04/15] soc: octeontx2: Add mailbox support infra
From: Sunil Kovvuri @ 2018-08-28 13:23 UTC (permalink / raw)
To: Arnd Bergmann
Cc: LKML, olof, LAKML, linux-soc, Aleksey Makarov, Sunil Goutham,
Lukasz Bartosik, Linux Netdev List, David S. Miller
In-Reply-To: <CAK8P3a1CvX1V8h20s_=XUUYfR-zYNwETg5p6dCtpVUqZ89K-tg@mail.gmail.com>
On Tue, Aug 28, 2018 at 6:22 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Aug 28, 2018 at 2:48 PM Sunil Kovvuri <sunil.kovvuri@gmail.com> wrote:
> >
> > On Tue, Aug 28, 2018 at 5:33 PM Arnd Bergmann <arnd@arndb.de> wrote:
> > >
> > > On Tue, Aug 28, 2018 at 12:57 PM <sunil.kovvuri@gmail.com> wrote:
> > > >
> > > > From: Aleksey Makarov <amakarov@marvell.com>
> > > >
> > > > This patch adds mailbox support infrastructure APIs.
> > > > Each RVU device has a dedicated 64KB mailbox region
> > > > shared with it's peer for communication. RVU AF has
> > > > a separate mailbox region shared with each of RVU PFs
> > > > and a RVU PF has a separate region shared with each of
> > > > it's VF.
> > > >
> > > > These set of APIs are used by this driver (RVU AF) and
> > > > other RVU PF/VF drivers eg netdev, crypto e.t.c.
> > > >
> > > > Signed-off-by: Aleksey Makarov <amakarov@marvell.com>
> > > > Signed-off-by: Sunil Goutham <sgoutham@marvell.com>
> > > > Signed-off-by: Lukasz Bartosik <lbartosik@marvell.com>
> > >
> > > Why does this driver not use the drivers/mailbox/ infrastructure?
> > >
> > This is a common administrative software driver which will be handling requests
> > from kernel drivers and as well as drivers in userspace applications.
> > We had to keep mailbox communication infrastructure same across all usages.
>
> Can you explain more about the usage of userspace applications
> and what interface you plan to use into the kernel?
Any PCI device here irrespective in what domain (kernel or userspace)
they are in
use common mailbox communication. Which is
# Write a mailbox msg (format is agreed between all parties) into
shared (between AF and other PF/VFs)
memory region and trigger a interrupt to admin function.
# Admin function processes the msg and puts reply in the same memory
region and trigger
IRQ to the requesting device. If the device has a driver instance
in kernel then it uses
IRQ and userspace applications does polling on the IRQ status bit.
>
> Do you things like AF_XDP and virtual machines, or something else?
I meant drivers in DPDK which may or may not use AF_XDP.
And yes if a PCI device is attached to a virtual machine then that
also uses the same mailbox communication.
Sunil.
>
> Arnd
^ permalink raw reply
* Re: [PATCH 00/15] soc: octeontx2: Add RVU admin function driver
From: Sunil Kovvuri @ 2018-08-28 13:24 UTC (permalink / raw)
To: Arnd Bergmann
Cc: LKML, olof, LAKML, linux-soc, Sunil Goutham, Linux Netdev List,
David S. Miller
In-Reply-To: <CAK8P3a1xC6XjwDAmocpE5TZO7j6uVGtvBtWLPVcYwKqN6Z+seQ@mail.gmail.com>
On Tue, Aug 28, 2018 at 5:53 PM Arnd Bergmann <arnd@arndb.de> wrote:
>
> On Tue, Aug 28, 2018 at 12:57 PM <sunil.kovvuri@gmail.com> wrote:
> >
> > From: Sunil Goutham <sgoutham@marvell.com>
> >
> > Resource virtualization unit (RVU) on Marvell's OcteonTX2 SOC supports
> > multiple PCIe SRIOV physical functions (PFs) and virtual functions (VFs).
> > PF0 is called administrative / admin function (AF) and has privilege access
> > to registers to provision different RVU functional blocks to each of
> > PF/VF.
> >
> > This admin function (AF) driver acts as a configuration / administrative
> > software which provisions functional blocks to a PF/VF on demand for them
> > to work as one of the following
> > - A basic network controller (i.e NIC).
> > - NIC with packet filtering, shaping and scheduling capabilities.
> > - A crypto device.
> > - A combination of above etc.
> >
> > PF/VFs communicate with admin function via a shared memory region.
> > This patch series adds logic for the following
> > - RVU AF driver with functional blocks provisioning support
> > - Mailbox infrastructure for communication between AF and PFs.
> > - CGX driver which provides information about physcial network
> > interfaces which AF processes and forwards required info to
> > PF/VF drivers.
> >
> > This is the first set of patches out of 70 odd patches.
> >
> > Note: This driver neither receives any data nor processes it i.e no I/O,
> > just does the hardware configuration.
>
> Hi Sunil,
>
> Thanks for posting this first series, I'm glad we're seeing support for this
> chip family making some progress.
Thanks.
>
> My feeling overall is that we need a review from the network driver
> folks more than the arm-soc team etc, and that maybe the driver
> as a whole should go into drivers/net/ethernet.
This driver doesn't handle any network IO and moreever this driver has to handle
configuration requests from crypto driver as well. There will be
separate network and
crypto drivers which will be upstreamed into drivers/net/ethernet and
drivers/crypto.
And in future silicons there will be different types of functional
blocks which will be
added into this resource virtualization unit (RVU). Hence i thought
this driver is not a
right fit in drivers/net/ethernet.
>
> We support some couple of similar hardware already that has
> both support for virtual functions and for crypto offload, including
> the Chelsio cxgb4, Mellanox mlx5, NXP DPAA and probably others,
I agree, but i guess that is done to support inline crypto.
Here this driver has to handle requests from normal crypto driver
(drivers/crypto) as well.
> and we need to ensure that the exposed interfaces are all
> compatible, and that you use the correct subsystems and
> in-kernel abstractions for thing that are common.
>
> Arnd
^ permalink raw reply
* Re: [RFC/PATCH] Add a socketoption IPV6_MULTICAST_ALL analogue to the IPV4 version
From: Andre Naujoks @ 2018-08-28 9:36 UTC (permalink / raw)
To: netdev; +Cc: 吉藤英明, David S. Miller, yoshfuji
In-Reply-To: <CAPA1RqCtkYE8is=kbANNGktgoiqKHZy+0wtE-spUwgq4V-_XTw@mail.gmail.com>
On 5/8/18 1:48 PM, 吉藤英明 wrote:
> Hi,
>
> 2018-05-08 15:41 GMT+09:00 Andre Naujoks <nautsch2@gmail.com>:
>> On 08.05.2018 08:31, 吉藤英明 wrote:
>>> Hi,
>>>
>>> 2018-05-08 15:03 GMT+09:00 Andre Naujoks <nautsch2@gmail.com>:
>>>> On 11.04.2018 13:02, Andre Naujoks wrote:
>>>>> Hi.
>>>>
>>>> Hi again.
>>>>
>>>> Since it has been a month now, I'd like to send a little "ping" on this subject.
>>>>
>>>> Is anything wrong with this? Or was it just bad timing?
Hi.
Anything new about this? I still think, this is a very useful addition.
If this is not wanted/needed, then I'll stop pinging this topic.
Regards
Andre
>>>
>>> I'm just curious... What kind of behaviour do you expect?
>>>
>>> Unless you explicitly join the group, you cannot get traffic for the group
>>> because of multicast filtering at device level (multicast fitlering) or at the
>>> switch level (MLD).
>>>
>>> If an application is interested in (several) multicast groups, it should
>>> explicitly join the group. So I cannot find valid (or meaningful) use-case.
>>
>> I expect only to receive the multicast traffic of groups I explicitly joined on that
>> socket. This is was the IPv4 version of this socket option already does. The problem
>> only exists if multiple groups are joined and the socket therefore has to be bound
>> to the "any"-address. Then we get traffic from all multicast groups joined by any(!)
>> process on the system (plus anything else on that IP-port).
>
> Okay I agree that we should be able NOT to get such traffic.
>
> Acked-By: YOSHIFUJI Hideaki <yoshfuji@linux-ipv6.org>
>
> --yoshfuji
>
>>
>> Regards
>> Andre
>>
>>>
>>> --yoshfuji
>>>
>>>>
>>>> Regards
>>>> Andre
>>>>
>>>>>
>>>>> I was running into a problem, when trying to join multiple multicast groups
>>>>> on a single socket and thus binding to the any-address on said socket. I
>>>>> received traffic from multicast groups, I did not join on that socket and
>>>>> was at first surprised by that. After reading some old e-mails/threads,
>>>>> which came to the conclusion "It is, as it is."
>>>>> (e.g https://marc.info/?l=linux-kernel&m=115815686626791&w=2), I discovered
>>>>> the IPv4 socketoption IP_MULTICAST_ALL, which, when disabled, does exactly
>>>>> what I would expect from a socket by default.
>>>>>
>>>>> I propose a socket option for IPv6, which does the same and has the same
>>>>> default as the IPv4 version. My first thought was, to just apply
>>>>> IP_MULTICAST_ALL to a ipv6 socket, but that would change the behavior of
>>>>> current applications and would probably be a big no-no.
>>>>>
>>>>> Regards
>>>>> Andre
>>>>>
>>>>>
>>>>> From 473653086c05a3de839c3504885053f6254c7bc5 Mon Sep 17 00:00:00 2001
>>>>> From: Andre Naujoks <nautsch2@gmail.com>
>>>>> Date: Wed, 11 Apr 2018 12:38:28 +0200
>>>>> Subject: [PATCH] Add a socketoption IPV6_MULTICAST_ALL analogue to the IPV4
>>>>> version
>>>>>
>>>>> The socket option will be enabled by default to ensure current behaviour
>>>>> is not changed. This is the same for the IPv4 version.
>>>>>
>>>>> A socket bound to in6addr_any and a specific port will receive all traffic
>>>>> on that port. Analogue to IP_MULTICAST_ALL, disable this behaviour, if
>>>>> one or more multicast groups were joined (using said socket) and only
>>>>> pass on multicast traffic from groups, which were explicitly joined via
>>>>> this socket.
>>>>>
>>>>> Without this option disabled a socket (system even) joined to multiple
>>>>> multicast groups is very hard to get right. Filtering by destination
>>>>> address has to take place in user space to avoid receiving multicast
>>>>> traffic from other multicast groups, which might have traffic on the same
>>>>> port.
>>>>>
>>>>> The extension of the IP_MULTICAST_ALL socketoption to just apply to ipv6,
>>>>> too, is not done to avoid changing the behaviour of current applications.
>>>>>
>>>>> Signed-off-by: Andre Naujoks <nautsch2@gmail.com>
>>>>> ---
>>>>> include/linux/ipv6.h | 3 ++-
>>>>> include/uapi/linux/in6.h | 1 +
>>>>> net/ipv6/af_inet6.c | 1 +
>>>>> net/ipv6/ipv6_sockglue.c | 11 +++++++++++
>>>>> net/ipv6/mcast.c | 2 +-
>>>>> 5 files changed, 16 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/include/linux/ipv6.h b/include/linux/ipv6.h
>>>>> index 8415bf1a9776..495e834c1367 100644
>>>>> --- a/include/linux/ipv6.h
>>>>> +++ b/include/linux/ipv6.h
>>>>> @@ -274,7 +274,8 @@ struct ipv6_pinfo {
>>>>> */
>>>>> dontfrag:1,
>>>>> autoflowlabel:1,
>>>>> - autoflowlabel_set:1;
>>>>> + autoflowlabel_set:1,
>>>>> + mc_all:1;
>>>>> __u8 min_hopcount;
>>>>> __u8 tclass;
>>>>> __be32 rcv_flowinfo;
>>>>> diff --git a/include/uapi/linux/in6.h b/include/uapi/linux/in6.h
>>>>> index ed291e55f024..71d82fe15b03 100644
>>>>> --- a/include/uapi/linux/in6.h
>>>>> +++ b/include/uapi/linux/in6.h
>>>>> @@ -177,6 +177,7 @@ struct in6_flowlabel_req {
>>>>> #define IPV6_V6ONLY 26
>>>>> #define IPV6_JOIN_ANYCAST 27
>>>>> #define IPV6_LEAVE_ANYCAST 28
>>>>> +#define IPV6_MULTICAST_ALL 29
>>>>>
>>>>> /* IPV6_MTU_DISCOVER values */
>>>>> #define IPV6_PMTUDISC_DONT 0
>>>>> diff --git a/net/ipv6/af_inet6.c b/net/ipv6/af_inet6.c
>>>>> index 8da0b513f188..7844cd9d2f10 100644
>>>>> --- a/net/ipv6/af_inet6.c
>>>>> +++ b/net/ipv6/af_inet6.c
>>>>> @@ -209,6 +209,7 @@ static int inet6_create(struct net *net, struct socket *sock, int protocol,
>>>>> np->hop_limit = -1;
>>>>> np->mcast_hops = IPV6_DEFAULT_MCASTHOPS;
>>>>> np->mc_loop = 1;
>>>>> + np->mc_all = 1;
>>>>> np->pmtudisc = IPV6_PMTUDISC_WANT;
>>>>> np->repflow = net->ipv6.sysctl.flowlabel_reflect;
>>>>> sk->sk_ipv6only = net->ipv6.sysctl.bindv6only;
>>>>> diff --git a/net/ipv6/ipv6_sockglue.c b/net/ipv6/ipv6_sockglue.c
>>>>> index 4d780c7f0130..b2bc1942a2ee 100644
>>>>> --- a/net/ipv6/ipv6_sockglue.c
>>>>> +++ b/net/ipv6/ipv6_sockglue.c
>>>>> @@ -664,6 +664,13 @@ static int do_ipv6_setsockopt(struct sock *sk, int level, int optname,
>>>>> retv = ipv6_sock_ac_drop(sk, mreq.ipv6mr_ifindex, &mreq.ipv6mr_acaddr);
>>>>> break;
>>>>> }
>>>>> + case IPV6_MULTICAST_ALL:
>>>>> + if (optlen < sizeof(int))
>>>>> + goto e_inval;
>>>>> + np->mc_all = valbool;
>>>>> + retv = 0;
>>>>> + break;
>>>>> +
>>>>> case MCAST_JOIN_GROUP:
>>>>> case MCAST_LEAVE_GROUP:
>>>>> {
>>>>> @@ -1255,6 +1262,10 @@ static int do_ipv6_getsockopt(struct sock *sk, int level, int optname,
>>>>> val = np->mcast_oif;
>>>>> break;
>>>>>
>>>>> + case IPV6_MULTICAST_ALL:
>>>>> + val = np->mc_all;
>>>>> + break;
>>>>> +
>>>>> case IPV6_UNICAST_IF:
>>>>> val = (__force int)htonl((__u32) np->ucast_oif);
>>>>> break;
>>>>> diff --git a/net/ipv6/mcast.c b/net/ipv6/mcast.c
>>>>> index 793159d77d8a..623ad00eb3c2 100644
>>>>> --- a/net/ipv6/mcast.c
>>>>> +++ b/net/ipv6/mcast.c
>>>>> @@ -622,7 +622,7 @@ bool inet6_mc_check(struct sock *sk, const struct in6_addr *mc_addr,
>>>>> }
>>>>> if (!mc) {
>>>>> rcu_read_unlock();
>>>>> - return true;
>>>>> + return np->mc_all;
>>>>> }
>>>>> read_lock(&mc->sflock);
>>>>> psl = mc->sflist;
>>>>>
>>>>
>>
^ permalink raw reply
* Re: [PATCH] ath10k: use struct_size() in kzalloc()
From: Kalle Valo @ 2018-08-28 13:43 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: David S. Miller, ath10k, linux-wireless, netdev, linux-kernel,
Kees Cook, Gustavo A. R. Silva
In-Reply-To: <20180824011247.GA25648@embeddedor.com>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> void *entry[];
> };
>
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
06ae8dc00433 ath10k: use struct_size() in kzalloc()
--
https://patchwork.kernel.org/patch/10574741/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] ath10k: use struct_size() in kzalloc()
From: Kalle Valo @ 2018-08-28 13:43 UTC (permalink / raw)
To: Gustavo A. R. Silva
Cc: Kees Cook, Gustavo A. R. Silva, netdev, linux-wireless,
linux-kernel, ath10k, David S. Miller
In-Reply-To: <20180824011247.GA25648@embeddedor.com>
"Gustavo A. R. Silva" <gustavo@embeddedor.com> wrote:
> One of the more common cases of allocation size calculations is finding
> the size of a structure that has a zero-sized array at the end, along
> with memory for some number of elements for that array. For example:
>
> struct foo {
> int stuff;
> void *entry[];
> };
>
> instance = kzalloc(sizeof(struct foo) + sizeof(void *) * count, GFP_KERNEL);
>
> Instead of leaving these open-coded and prone to type mistakes, we can
> now use the new struct_size() helper:
>
> instance = kzalloc(struct_size(instance, entry, count), GFP_KERNEL);
>
> This issue was detected with the help of Coccinelle.
>
> Signed-off-by: Gustavo A. R. Silva <gustavo@embeddedor.com>
> Reviewed-by: Kees Cook <keescook@chromium.org>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
06ae8dc00433 ath10k: use struct_size() in kzalloc()
--
https://patchwork.kernel.org/patch/10574741/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] net: wireless: ath9k: Remove unnecessary parentheses
From: Kalle Valo @ 2018-08-28 13:44 UTC (permalink / raw)
To: Varsha Rao
Cc: Lukas Bulwahn, Nicholas Mc Guire, QCA ath9k Development,
David S. Miller, linux-wireless, netdev, linux-kernel, Varsha Rao
In-Reply-To: <20180725185305.4118-1-rvarsha016@gmail.com>
Varsha Rao <rvarsha016@gmail.com> wrote:
> Remove extra parentheses to fix the clang warning of extraneous
> parentheses.
>
> Signed-off-by: Varsha Rao <rvarsha016@gmail.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
bf05e0fe7da4 ath9k: Remove unnecessary parentheses
--
https://patchwork.kernel.org/patch/10544571/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH] ath9k: remove unused array firstep_table
From: Kalle Valo @ 2018-08-28 13:45 UTC (permalink / raw)
To: Colin King
Cc: QCA ath9k Development, David S . Miller, linux-wireless, netdev,
kernel-janitors, linux-kernel
In-Reply-To: <20180816125030.4416-1-colin.king@canonical.com>
Colin King <colin.king@canonical.com> wrote:
> Array firstep_table is being assigned but is never used
> hence it is redundant and can be removed.
>
> Cleans up clang warning:
> warning: 'firstep_table' defined but not used [-Wunused-const-variable=]
>
> Signed-off-by: Colin Ian King <colin.king@canonical.com>
> Signed-off-by: Kalle Valo <kvalo@codeaurora.org>
Patch applied to ath-next branch of ath.git, thanks.
a2f73a167dc1 ath9k: remove unused array firstep_table
--
https://patchwork.kernel.org/patch/10567385/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
^ permalink raw reply
* Re: [PATCH v2 01/29] nvmem: add support for cell lookups
From: Srinivas Kandagatla @ 2018-08-28 13:45 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Boris Brezillon, Andrew Lunn, linux-doc, Sekhar Nori,
Bartosz Golaszewski, linux-i2c, Mauro Carvalho Chehab,
Rob Herring, Florian Fainelli, Kevin Hilman, Richard Weinberger,
Russell King, Marek Vasut, Paolo Abeni, Dan Carpenter,
Grygorii Strashko, David Lechner, Arnd Bergmann,
Sven Van Asbroeck, "open list:
In-Reply-To: <CAMRc=Mex5WAm8uiV4T=0+bLduBBdj18iekuYT55W9gA_rzQ_sg@mail.gmail.com>
On 28/08/18 12:56, Bartosz Golaszewski wrote:
> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
>>
>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
>>>
>>> I didn't notice it before but there's a global list of nvmem cells
>>
>>
>> Bit of history here.
>>
>> The global list of nvmem_cell is to assist non device tree based cell
>> lookups. These cell entries come as part of the non-dt providers
>> nvmem_config.
>>
>> All the device tree based cell lookup happen dynamically on request/demand,
>> and all the cell definition comes from DT.
>>
>
> Makes perfect sense.
>
>> As of today NVMEM supports both DT and non DT usecase, this is much simpler.
>>
>> Non dt cases have various consumer usecases.
>>
>> 1> Consumer is aware of provider name and cell details.
>> This is probably simple usecase where it can just use device based
>> apis.
>>
>> 2> Consumer is not aware of provider name, its just aware of cell name.
>> This is the case where global list of cells are used.
>>
>
> I would like to support an additional use case here: the provider is
> generic and is not aware of its cells at all. Since the only way of
> defining nvmem cells is through DT or nvmem_config, we lack a way to
> allow machine code to define cells without the provider code being
> aware.
machine driver should be able to do
nvmem_device_get()
nvmem_add_cells()
currently this adds to the global cell list which is exactly like doing
it via nvmem_config.
>
>>> with each cell referencing its owner nvmem device. I'm wondering if
>>> this isn't some kind of inversion of ownership. Shouldn't each nvmem
>>> device have a separate list of nvmem cells owned by it? What happens
>>
>> This is mainly done for use case where consumer does not have idea of
>> provider name or any details.
>>
>
> It doesn't need to know the provider details, but in most subsystems
> the core code associates such resources by dev_id and optional con_id
> as Boris already said.
>
If dev_id here is referring to provider dev_id, then we already do that
using nvmem device apis, except in global cell list which makes dev_id
optional.
>> First thing non dt user should do is use "NVMEM device based consumer APIs"
>>
>> ex: First get handle to nvmem device using its nvmem provider name by
>> calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.
>>
>> Also am not 100% sure how would maintaining cells list per nvmem provider
>> would help for the intended purpose of global list?
>>
>
> It would fix the use case where the consumer wants to use
> nvmem_cell_get(dev, name) and two nvmem providers would have a cell
> with the same name.
There is no code to enforce duplicate checks, so this would just
decrease the chances rather than fixing the problem totally.
I guess this is same problem
Finding cell by name without dev_id would still be an issue, am not too
concerned about this ATM.
However, the idea of having cells per provider does sound good to me.
We should also maintain list of providers in core as a lookup in cases
where dev_id is null.
I did hack up a patch, incase you might want to try:
I did only compile test.
---------------------------------->cut<-------------------------------
Author: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
Date: Tue Aug 28 13:46:21 2018 +0100
nvmem: core: maintain per provider cell list
Having a global cell list could be a issue in cases where the
cell-id is same across multiple providers. Making the cell list specific
to provider could avoid such issue by adding additional checks while
addding cells.
Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
index aa1657831b70..29da603f2fa4 100644
--- a/drivers/nvmem/core.c
+++ b/drivers/nvmem/core.c
@@ -40,6 +40,8 @@ struct nvmem_device {
struct device *base_dev;
nvmem_reg_read_t reg_read;
nvmem_reg_write_t reg_write;
+ struct list_head node;
+ struct list_head cells;
void *priv;
};
@@ -57,9 +59,7 @@ struct nvmem_cell {
static DEFINE_MUTEX(nvmem_mutex);
static DEFINE_IDA(nvmem_ida);
-
-static LIST_HEAD(nvmem_cells);
-static DEFINE_MUTEX(nvmem_cells_mutex);
+static LIST_HEAD(nvmem_devices);
#ifdef CONFIG_DEBUG_LOCK_ALLOC
static struct lock_class_key eeprom_lock_key;
@@ -284,26 +284,28 @@ static struct nvmem_device *of_nvmem_find(struct
device_node *nvmem_np)
static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
{
- struct nvmem_cell *p;
+ struct nvmem_device *d;
- mutex_lock(&nvmem_cells_mutex);
-
- list_for_each_entry(p, &nvmem_cells, node)
- if (!strcmp(p->name, cell_id)) {
- mutex_unlock(&nvmem_cells_mutex);
- return p;
- }
+ mutex_lock(&nvmem_mutex);
+ list_for_each_entry(d, &nvmem_devices, node) {
+ struct nvmem_cell *p;
+ list_for_each_entry(p, &d->cells, node)
+ if (!strcmp(p->name, cell_id)) {
+ mutex_unlock(&nvmem_mutex);
+ return p;
+ }
+ }
- mutex_unlock(&nvmem_cells_mutex);
+ mutex_unlock(&nvmem_mutex);
return NULL;
}
static void nvmem_cell_drop(struct nvmem_cell *cell)
{
- mutex_lock(&nvmem_cells_mutex);
+ mutex_lock(&nvmem_mutex);
list_del(&cell->node);
- mutex_unlock(&nvmem_cells_mutex);
+ mutex_unlock(&nvmem_mutex);
kfree(cell);
}
@@ -312,18 +314,18 @@ static void nvmem_device_remove_all_cells(const
struct nvmem_device *nvmem)
struct nvmem_cell *cell;
struct list_head *p, *n;
- list_for_each_safe(p, n, &nvmem_cells) {
+ list_for_each_safe(p, n, &nvmem->cells) {
cell = list_entry(p, struct nvmem_cell, node);
if (cell->nvmem == nvmem)
nvmem_cell_drop(cell);
}
}
-static void nvmem_cell_add(struct nvmem_cell *cell)
+static void nvmem_cell_add(struct nvmem_device *nvmem, struct
nvmem_cell *cell)
{
- mutex_lock(&nvmem_cells_mutex);
- list_add_tail(&cell->node, &nvmem_cells);
- mutex_unlock(&nvmem_cells_mutex);
+ mutex_lock(&nvmem_mutex);
+ list_add_tail(&cell->node, &nvmem->cells);
+ mutex_unlock(&nvmem_mutex);
}
static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
@@ -385,7 +387,7 @@ int nvmem_add_cells(struct nvmem_device *nvmem,
goto err;
}
- nvmem_cell_add(cells[i]);
+ nvmem_cell_add(nvmem, cells[i]);
}
/* remove tmp array */
@@ -519,6 +521,10 @@ struct nvmem_device *nvmem_register(const struct
nvmem_config *config)
if (config->cells)
nvmem_add_cells(nvmem, config->cells, config->ncells);
+ mutex_lock(&nvmem_mutex);
+ list_add_tail(&nvmem->node, &nvmem_devices);
+ mutex_unlock(&nvmem_mutex);
+
return nvmem;
err_device_del:
@@ -544,6 +550,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
mutex_unlock(&nvmem_mutex);
return -EBUSY;
}
+
+ list_del(&nvmem->node);
mutex_unlock(&nvmem_mutex);
if (nvmem->flags & FLAG_COMPAT)
@@ -899,7 +907,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct
device_node *np,
goto err_sanity;
}
- nvmem_cell_add(cell);
+ nvmem_cell_add(nvmem, cell);
return cell;
---------------------------------->cut<-------------------------------
>
> Next we could add a way to associate dev_ids with nvmem cells.
>
>>> if we have two nvmem providers with the same names for cells? I'm
>>
>> Yes, it would return the first instance.. which is a known issue.
>> Am not really sure this is a big problem as of today! but am open for any
>> better suggestions!
>>
>
> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
> defining nvmem-cells using nvmem_config. I think that what we need is
> a way of specifying cell config outside of nvmem providers in some
> kind of structures. These tables would reference the provider by name
> and define the cells. Then we would have an additional lookup
> structure which would associate the consumer (by dev_id and con_id,
> where dev_id could optionally be NULL and where we would fall back to
> using con_id only) and the nvmem provider + cell together. Similarly
> to how GPIO consumers are associated with the gpiochip and hwnum. How
> does it sound?
Yes, sounds good.
Correct me if am wrong!
You should be able to add the new cells using struct nvmem_cell_info and
add them to particular provider using nvmem_add_cells().
Sounds like thats exactly what nvmem_add_lookup_table() would look like.
We should add new nvmem_device_cell_get(nvmem, conn_id) which would
return nvmem cell which is specific to the provider. This cell can be
used by the machine driver to read/write.
>>>
>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>> instance even if the cell for this node was already added to the nvmem
>>> device.
>>
>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>> instance for every get!!
>
>
> I admit I didn't test it, but just from reading the code it seems like
> in nvmem_cell_get() for DT-users we'll always get to
> of_nvmem_cell_get() and in there we always end up calling line 873:
> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>
That is correct, this cell is created when we do a get and release when
we do a put().
thanks,
srini
^ permalink raw reply related
* RV: 20183516 document set
From: Docs @ 2018-08-28 8:41 UTC (permalink / raw)
To: netdev
[-- Attachment #1: Type: text/plain, Size: 21 bytes --]
DEAR ALL
TAKE NOTE
[-- Attachment #2: 20183516 document set.xlsx --]
[-- Type: application/octet-stream, Size: 81752 bytes --]
^ permalink raw reply
* Re: [PATCH v2 01/29] nvmem: add support for cell lookups
From: Bartosz Golaszewski @ 2018-08-28 14:41 UTC (permalink / raw)
To: Srinivas Kandagatla
Cc: Boris Brezillon, Andrew Lunn, linux-doc, Sekhar Nori,
Bartosz Golaszewski, linux-i2c, Mauro Carvalho Chehab,
Rob Herring, Florian Fainelli, Kevin Hilman, Richard Weinberger,
Russell King, Marek Vasut, Paolo Abeni, Dan Carpenter,
Grygorii Strashko, David Lechner, Arnd Bergmann,
Sven Van Asbroeck, "open list:
In-Reply-To: <916e3e89-82b3-0d52-2b77-4374261a9d0f@linaro.org>
2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
>
>
> On 28/08/18 12:56, Bartosz Golaszewski wrote:
>>
>> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla
>> <srinivas.kandagatla@linaro.org>:
>>>
>>>
>>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
>>>>
>>>>
>>>> I didn't notice it before but there's a global list of nvmem cells
>>>
>>>
>>>
>>> Bit of history here.
>>>
>>> The global list of nvmem_cell is to assist non device tree based cell
>>> lookups. These cell entries come as part of the non-dt providers
>>> nvmem_config.
>>>
>>> All the device tree based cell lookup happen dynamically on
>>> request/demand,
>>> and all the cell definition comes from DT.
>>>
>>
>> Makes perfect sense.
>>
>>> As of today NVMEM supports both DT and non DT usecase, this is much
>>> simpler.
>>>
>>> Non dt cases have various consumer usecases.
>>>
>>> 1> Consumer is aware of provider name and cell details.
>>> This is probably simple usecase where it can just use device
>>> based
>>> apis.
>>>
>>> 2> Consumer is not aware of provider name, its just aware of cell name.
>>> This is the case where global list of cells are used.
>>>
>>
>> I would like to support an additional use case here: the provider is
>> generic and is not aware of its cells at all. Since the only way of
>> defining nvmem cells is through DT or nvmem_config, we lack a way to
>> allow machine code to define cells without the provider code being
>> aware.
>
>
> machine driver should be able to do
> nvmem_device_get()
> nvmem_add_cells()
>
Indeed, I missed the fact that you can retrieve the nvmem device by
name. Except that we cannot know that the nvmem provider has been
registered yet when calling nvmem_device_get(). This could potentially
be solved by my other patch that adds notifiers to nvmem, but it would
require much more boilerplate code in every board file. I think that
removing nvmem_cell_info from nvmem_config and having external cell
definitions would be cleaner.
> currently this adds to the global cell list which is exactly like doing it
> via nvmem_config.
>>
>>
>>>> with each cell referencing its owner nvmem device. I'm wondering if
>>>> this isn't some kind of inversion of ownership. Shouldn't each nvmem
>>>> device have a separate list of nvmem cells owned by it? What happens
>>>
>>>
>>> This is mainly done for use case where consumer does not have idea of
>>> provider name or any details.
>>>
>>
>> It doesn't need to know the provider details, but in most subsystems
>> the core code associates such resources by dev_id and optional con_id
>> as Boris already said.
>>
>
> If dev_id here is referring to provider dev_id, then we already do that
> using nvmem device apis, except in global cell list which makes dev_id
> optional.
>
>
>>> First thing non dt user should do is use "NVMEM device based consumer
>>> APIs"
>>>
>>> ex: First get handle to nvmem device using its nvmem provider name by
>>> calling nvmem_device_get(); and use nvmem_device_cell_read/write() apis.
>>>
>>> Also am not 100% sure how would maintaining cells list per nvmem provider
>>> would help for the intended purpose of global list?
>>>
>>
>> It would fix the use case where the consumer wants to use
>> nvmem_cell_get(dev, name) and two nvmem providers would have a cell
>> with the same name.
>
>
> There is no code to enforce duplicate checks, so this would just decrease
> the chances rather than fixing the problem totally.
> I guess this is same problem
>
> Finding cell by name without dev_id would still be an issue, am not too
> concerned about this ATM.
>
> However, the idea of having cells per provider does sound good to me.
> We should also maintain list of providers in core as a lookup in cases where
> dev_id is null.
>
> I did hack up a patch, incase you might want to try:
> I did only compile test.
> ---------------------------------->cut<-------------------------------
> Author: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
> Date: Tue Aug 28 13:46:21 2018 +0100
>
> nvmem: core: maintain per provider cell list
>
> Having a global cell list could be a issue in cases where the cell-id is
> same across multiple providers. Making the cell list specific to provider
> could avoid such issue by adding additional checks while addding cells.
>
> Signed-off-by: Srinivas Kandagatla <srinivas.kandagatla@linaro.org>
>
> diff --git a/drivers/nvmem/core.c b/drivers/nvmem/core.c
> index aa1657831b70..29da603f2fa4 100644
> --- a/drivers/nvmem/core.c
> +++ b/drivers/nvmem/core.c
> @@ -40,6 +40,8 @@ struct nvmem_device {
> struct device *base_dev;
> nvmem_reg_read_t reg_read;
> nvmem_reg_write_t reg_write;
> + struct list_head node;
> + struct list_head cells;
> void *priv;
> };
>
> @@ -57,9 +59,7 @@ struct nvmem_cell {
>
> static DEFINE_MUTEX(nvmem_mutex);
> static DEFINE_IDA(nvmem_ida);
> -
> -static LIST_HEAD(nvmem_cells);
> -static DEFINE_MUTEX(nvmem_cells_mutex);
> +static LIST_HEAD(nvmem_devices);
>
> #ifdef CONFIG_DEBUG_LOCK_ALLOC
> static struct lock_class_key eeprom_lock_key;
> @@ -284,26 +284,28 @@ static struct nvmem_device *of_nvmem_find(struct
> device_node *nvmem_np)
>
> static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
> {
> - struct nvmem_cell *p;
> + struct nvmem_device *d;
>
> - mutex_lock(&nvmem_cells_mutex);
> -
> - list_for_each_entry(p, &nvmem_cells, node)
> - if (!strcmp(p->name, cell_id)) {
> - mutex_unlock(&nvmem_cells_mutex);
> - return p;
> - }
> + mutex_lock(&nvmem_mutex);
> + list_for_each_entry(d, &nvmem_devices, node) {
> + struct nvmem_cell *p;
> + list_for_each_entry(p, &d->cells, node)
> + if (!strcmp(p->name, cell_id)) {
> + mutex_unlock(&nvmem_mutex);
> + return p;
> + }
> + }
>
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_unlock(&nvmem_mutex);
>
> return NULL;
> }
>
> static void nvmem_cell_drop(struct nvmem_cell *cell)
> {
> - mutex_lock(&nvmem_cells_mutex);
> + mutex_lock(&nvmem_mutex);
> list_del(&cell->node);
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_unlock(&nvmem_mutex);
> kfree(cell);
> }
>
> @@ -312,18 +314,18 @@ static void nvmem_device_remove_all_cells(const struct
> nvmem_device *nvmem)
> struct nvmem_cell *cell;
> struct list_head *p, *n;
>
> - list_for_each_safe(p, n, &nvmem_cells) {
> + list_for_each_safe(p, n, &nvmem->cells) {
> cell = list_entry(p, struct nvmem_cell, node);
> if (cell->nvmem == nvmem)
> nvmem_cell_drop(cell);
> }
> }
>
> -static void nvmem_cell_add(struct nvmem_cell *cell)
> +static void nvmem_cell_add(struct nvmem_device *nvmem, struct nvmem_cell
> *cell)
> {
> - mutex_lock(&nvmem_cells_mutex);
> - list_add_tail(&cell->node, &nvmem_cells);
> - mutex_unlock(&nvmem_cells_mutex);
> + mutex_lock(&nvmem_mutex);
> + list_add_tail(&cell->node, &nvmem->cells);
> + mutex_unlock(&nvmem_mutex);
> }
>
> static int nvmem_cell_info_to_nvmem_cell(struct nvmem_device *nvmem,
> @@ -385,7 +387,7 @@ int nvmem_add_cells(struct nvmem_device *nvmem,
> goto err;
> }
>
> - nvmem_cell_add(cells[i]);
> + nvmem_cell_add(nvmem, cells[i]);
> }
>
> /* remove tmp array */
> @@ -519,6 +521,10 @@ struct nvmem_device *nvmem_register(const struct
> nvmem_config *config)
> if (config->cells)
> nvmem_add_cells(nvmem, config->cells, config->ncells);
>
> + mutex_lock(&nvmem_mutex);
> + list_add_tail(&nvmem->node, &nvmem_devices);
> + mutex_unlock(&nvmem_mutex);
> +
> return nvmem;
>
> err_device_del:
> @@ -544,6 +550,8 @@ int nvmem_unregister(struct nvmem_device *nvmem)
> mutex_unlock(&nvmem_mutex);
> return -EBUSY;
> }
> +
> + list_del(&nvmem->node);
> mutex_unlock(&nvmem_mutex);
>
> if (nvmem->flags & FLAG_COMPAT)
> @@ -899,7 +907,7 @@ struct nvmem_cell *of_nvmem_cell_get(struct device_node
> *np,
> goto err_sanity;
> }
>
> - nvmem_cell_add(cell);
> + nvmem_cell_add(nvmem, cell);
>
> return cell;
>
> ---------------------------------->cut<-------------------------------
>
>>
>> Next we could add a way to associate dev_ids with nvmem cells.
>>
>>>> if we have two nvmem providers with the same names for cells? I'm
>>>
>>>
>>> Yes, it would return the first instance.. which is a known issue.
>>> Am not really sure this is a big problem as of today! but am open for any
>>> better suggestions!
>>>
>>
>> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
>> defining nvmem-cells using nvmem_config. I think that what we need is
>> a way of specifying cell config outside of nvmem providers in some
>> kind of structures. These tables would reference the provider by name
>> and define the cells. Then we would have an additional lookup
>> structure which would associate the consumer (by dev_id and con_id,
>> where dev_id could optionally be NULL and where we would fall back to
>> using con_id only) and the nvmem provider + cell together. Similarly
>> to how GPIO consumers are associated with the gpiochip and hwnum. How
>> does it sound?
>
> Yes, sounds good.
>
> Correct me if am wrong!
> You should be able to add the new cells using struct nvmem_cell_info and add
> them to particular provider using nvmem_add_cells().
>
> Sounds like thats exactly what nvmem_add_lookup_table() would look like.
>
> We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
> nvmem cell which is specific to the provider. This cell can be used by the
> machine driver to read/write.
Except that we could do it lazily - when the nvmem provider actually
gets registered instead of doing it right away and risking that the
device isn't even there yet.
>
>>>>
>>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>>> instance even if the cell for this node was already added to the nvmem
>>>> device.
>>>
>>>
>>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>>> instance for every get!!
>>
>>
>>
>> I admit I didn't test it, but just from reading the code it seems like
>> in nvmem_cell_get() for DT-users we'll always get to
>> of_nvmem_cell_get() and in there we always end up calling line 873:
>> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>>
> That is correct, this cell is created when we do a get and release when we
> do a put().
>
Shouldn't we add the cell to the list, and check first if it's there
and only create it if not?
Bart
^ permalink raw reply
* Re: [PATCH v2 01/29] nvmem: add support for cell lookups
From: Srinivas Kandagatla @ 2018-08-28 14:48 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Boris Brezillon, Andrew Lunn, linux-doc, Sekhar Nori,
Bartosz Golaszewski, linux-i2c, Mauro Carvalho Chehab,
Rob Herring, Florian Fainelli, Kevin Hilman, Richard Weinberger,
Russell King, Marek Vasut, Paolo Abeni, Dan Carpenter,
Grygorii Strashko, David Lechner, Arnd Bergmann,
Sven Van Asbroeck, "open list:
In-Reply-To: <CAMRc=MeALxpyBQhWxGPt9BubO_ZwKGih1d8zhRzFYZ0+3kHFAA@mail.gmail.com>
On 28/08/18 15:41, Bartosz Golaszewski wrote:
> 2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
>>
>>
...
>>> I would like to support an additional use case here: the provider is
>>> generic and is not aware of its cells at all. Since the only way of
>>> defining nvmem cells is through DT or nvmem_config, we lack a way to
>>> allow machine code to define cells without the provider code being
>>> aware.
>>
>>
>> machine driver should be able to do
>> nvmem_device_get()
>> nvmem_add_cells()
>>
>
> Indeed, I missed the fact that you can retrieve the nvmem device by
> name. Except that we cannot know that the nvmem provider has been
> registered yet when calling nvmem_device_get(). This could potentially
> be solved by my other patch that adds notifiers to nvmem, but it would
> require much more boilerplate code in every board file. I think that
> removing nvmem_cell_info from nvmem_config and having external cell
> definitions would be cleaner.
Yes, notifiers would work!
...
>>>
>>> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
>>> defining nvmem-cells using nvmem_config. I think that what we need is
>>> a way of specifying cell config outside of nvmem providers in some
>>> kind of structures. These tables would reference the provider by name
>>> and define the cells. Then we would have an additional lookup
>>> structure which would associate the consumer (by dev_id and con_id,
>>> where dev_id could optionally be NULL and where we would fall back to
>>> using con_id only) and the nvmem provider + cell together. Similarly
>>> to how GPIO consumers are associated with the gpiochip and hwnum. How
>>> does it sound?
>>
>> Yes, sounds good.
>>
>> Correct me if am wrong!
>> You should be able to add the new cells using struct nvmem_cell_info and add
>> them to particular provider using nvmem_add_cells().
>>
>> Sounds like thats exactly what nvmem_add_lookup_table() would look like.
>>
>> We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
>> nvmem cell which is specific to the provider. This cell can be used by the
>> machine driver to read/write.
>
> Except that we could do it lazily - when the nvmem provider actually
> gets registered instead of doing it right away and risking that the
> device isn't even there yet.
>
Yes, it makes more sense to do it once the provider is actually present!
>>
>>>>>
>>>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
>>>>> instance even if the cell for this node was already added to the nvmem
>>>>> device.
>>>>
>>>>
>>>> I hope you got the reason why of_nvmem_cell_get() always allocates new
>>>> instance for every get!!
>>>
>>>
>>>
>>> I admit I didn't test it, but just from reading the code it seems like
>>> in nvmem_cell_get() for DT-users we'll always get to
>>> of_nvmem_cell_get() and in there we always end up calling line 873:
>>> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
>>>
>> That is correct, this cell is created when we do a get and release when we
>> do a put().
>>
>
> Shouldn't we add the cell to the list, and check first if it's there
> and only create it if not?
Yes I agree, duplicate entry checks are missing!
--srini
>
> Bart
>
^ permalink raw reply
* Re: [PATCH v2 01/29] nvmem: add support for cell lookups
From: Boris Brezillon @ 2018-08-28 14:53 UTC (permalink / raw)
To: Bartosz Golaszewski
Cc: Srinivas Kandagatla, Andrew Lunn, linux-doc, Sekhar Nori,
Bartosz Golaszewski, linux-i2c, Mauro Carvalho Chehab,
Rob Herring, Florian Fainelli, Kevin Hilman, Richard Weinberger,
Russell King, Marek Vasut, Paolo Abeni, Dan Carpenter,
Grygorii Strashko, David Lechner, Arnd Bergmann,
Sven Van Asbroeck, "ope
In-Reply-To: <CAMRc=MeALxpyBQhWxGPt9BubO_ZwKGih1d8zhRzFYZ0+3kHFAA@mail.gmail.com>
On Tue, 28 Aug 2018 16:41:04 +0200
Bartosz Golaszewski <brgl@bgdev.pl> wrote:
> 2018-08-28 15:45 GMT+02:00 Srinivas Kandagatla <srinivas.kandagatla@linaro.org>:
> >
> >
> > On 28/08/18 12:56, Bartosz Golaszewski wrote:
> >>
> >> 2018-08-28 12:15 GMT+02:00 Srinivas Kandagatla
> >> <srinivas.kandagatla@linaro.org>:
> >>>
> >>>
> >>> On 27/08/18 14:37, Bartosz Golaszewski wrote:
> >>>>
> >>>>
> >>>> I didn't notice it before but there's a global list of nvmem cells
> >>>
> >>>
> >>>
> >>> Bit of history here.
> >>>
> >>> The global list of nvmem_cell is to assist non device tree based cell
> >>> lookups. These cell entries come as part of the non-dt providers
> >>> nvmem_config.
> >>>
> >>> All the device tree based cell lookup happen dynamically on
> >>> request/demand,
> >>> and all the cell definition comes from DT.
> >>>
> >>
> >> Makes perfect sense.
> >>
> >>> As of today NVMEM supports both DT and non DT usecase, this is much
> >>> simpler.
> >>>
> >>> Non dt cases have various consumer usecases.
> >>>
> >>> 1> Consumer is aware of provider name and cell details.
> >>> This is probably simple usecase where it can just use device
> >>> based
> >>> apis.
> >>>
> >>> 2> Consumer is not aware of provider name, its just aware of cell name.
> >>> This is the case where global list of cells are used.
> >>>
> >>
> >> I would like to support an additional use case here: the provider is
> >> generic and is not aware of its cells at all. Since the only way of
> >> defining nvmem cells is through DT or nvmem_config, we lack a way to
> >> allow machine code to define cells without the provider code being
> >> aware.
> >
> >
> > machine driver should be able to do
> > nvmem_device_get()
> > nvmem_add_cells()
> >
>
> Indeed, I missed the fact that you can retrieve the nvmem device by
> name. Except that we cannot know that the nvmem provider has been
> registered yet when calling nvmem_device_get(). This could potentially
> be solved by my other patch that adds notifiers to nvmem, but it would
> require much more boilerplate code in every board file. I think that
> removing nvmem_cell_info from nvmem_config and having external cell
> definitions would be cleaner.
I also vote for this option.
> >
> > static struct nvmem_cell *nvmem_find_cell(const char *cell_id)
Can we get rid of this function and just have the the version that
takes an nvmem_name and a cell_id.
> >> Yes, I would like to rework nvmem a bit. I don't see any non-DT users
> >> defining nvmem-cells using nvmem_config. I think that what we need is
> >> a way of specifying cell config outside of nvmem providers in some
> >> kind of structures. These tables would reference the provider by name
> >> and define the cells. Then we would have an additional lookup
> >> structure which would associate the consumer (by dev_id and con_id,
> >> where dev_id could optionally be NULL and where we would fall back to
> >> using con_id only) and the nvmem provider + cell together. Similarly
> >> to how GPIO consumers are associated with the gpiochip and hwnum. How
> >> does it sound?
> >
> > Yes, sounds good.
> >
> > Correct me if am wrong!
> > You should be able to add the new cells using struct nvmem_cell_info and add
> > them to particular provider using nvmem_add_cells().
> >
> > Sounds like thats exactly what nvmem_add_lookup_table() would look like.
> >
> > We should add new nvmem_device_cell_get(nvmem, conn_id) which would return
> > nvmem cell which is specific to the provider. This cell can be used by the
> > machine driver to read/write.
>
> Except that we could do it lazily - when the nvmem provider actually
> gets registered instead of doing it right away and risking that the
> device isn't even there yet.
And again, I agree with you. That's basically what lookup tables are
meant for: defining resources that are supposed to be attached to a
device when it's registered to a subsystem.
>
> >
> >>>>
> >>>> BTW: of_nvmem_cell_get() seems to always allocate an nvmem_cell
> >>>> instance even if the cell for this node was already added to the nvmem
> >>>> device.
> >>>
> >>>
> >>> I hope you got the reason why of_nvmem_cell_get() always allocates new
> >>> instance for every get!!
> >>
> >>
> >>
> >> I admit I didn't test it, but just from reading the code it seems like
> >> in nvmem_cell_get() for DT-users we'll always get to
> >> of_nvmem_cell_get() and in there we always end up calling line 873:
> >> cell = kzalloc(sizeof(*cell), GFP_KERNEL);
> >>
> > That is correct, this cell is created when we do a get and release when we
> > do a put().
> >
>
> Shouldn't we add the cell to the list, and check first if it's there
> and only create it if not?
Or even better: create the cells at registration time so that the
search code is the same for both DT and non-DT cases. Only the
registration would differ (with one path parsing the DT, and the other
one searching for nvmem cells defined with a nvmem-provider-lookup
table).
^ permalink raw reply
* [PATCH 0/3] xen-netback: hash mapping hanling adjustments
From: Jan Beulich @ 2018-08-28 14:54 UTC (permalink / raw)
To: Paul Durrant, Wei Liu; +Cc: xen-devel, davem, netdev
First and foremost the fix for XSA-270. On top of that further changes
which looked desirable to me while investigating that XSA.
1: fix input validation in xenvif_set_hash_mapping()
2: validate queue numbers in xenvif_set_hash_mapping()
3: handle page straddling in xenvif_set_hash_mapping()
Signed-off-by: Jan Beulich <jbeulich@suse.com>
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* [PATCH 1/3] xen-netback: fix input validation in xenvif_set_hash_mapping()
From: Jan Beulich @ 2018-08-28 14:59 UTC (permalink / raw)
To: Paul Durrant, Wei Liu; +Cc: xen-devel, davem, netdev
In-Reply-To: <5B85622302000078001E2A35@prv1-mh.provo.novell.com>
Both len and off are frontend specified values, so we need to make
sure there's no overflow when adding the two for the bounds check. We
also want to avoid undefined behavior and hence use off to index into
->hash.mapping[] only after bounds checking. This at the same time
allows to take care of not applying off twice for the bounds checking
against vif->num_queues.
It is also insufficient to bounds check copy_op.len, as this is len
truncated to 16 bits.
This is XSA-270 / CVE-2018-15471.
Reported-by: Felix Wilhelm <fwilhelm@google.com>
Signed-off-by: Jan Beulich <jbeulich@suse.com>
Reviewed-by: Paul Durrant <paul.durrant@citrix.com>
Tested-by: Paul Durrant <paul.durrant@citrix.com>
Cc: stable@vger.kernel.org [4.7 onwards]
---
drivers/net/xen-netback/hash.c | 12 +++++++-----
1 file changed, 7 insertions(+), 5 deletions(-)
--- 4.19-rc1-xen-netback-set-hash-mapping.orig/drivers/net/xen-netback/hash.c
+++ 4.19-rc1-xen-netback-set-hash-mapping/drivers/net/xen-netback/hash.c
@@ -332,20 +332,22 @@ u32 xenvif_set_hash_mapping_size(struct
u32 xenvif_set_hash_mapping(struct xenvif *vif, u32 gref, u32 len,
u32 off)
{
- u32 *mapping = &vif->hash.mapping[off];
+ u32 *mapping = vif->hash.mapping;
struct gnttab_copy copy_op = {
.source.u.ref = gref,
.source.domid = vif->domid,
- .dest.u.gmfn = virt_to_gfn(mapping),
.dest.domid = DOMID_SELF,
- .dest.offset = xen_offset_in_page(mapping),
- .len = len * sizeof(u32),
+ .len = len * sizeof(*mapping),
.flags = GNTCOPY_source_gref
};
- if ((off + len > vif->hash.size) || copy_op.len > XEN_PAGE_SIZE)
+ if ((off + len < off) || (off + len > vif->hash.size) ||
+ len > XEN_PAGE_SIZE / sizeof(*mapping))
return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
+ copy_op.dest.u.gmfn = virt_to_gfn(mapping + off);
+ copy_op.dest.offset = xen_offset_in_page(mapping + off);
+
while (len-- != 0)
if (mapping[off++] >= vif->num_queues)
return XEN_NETIF_CTRL_STATUS_INVALID_PARAMETER;
_______________________________________________
Xen-devel mailing list
Xen-devel@lists.xenproject.org
https://lists.xenproject.org/mailman/listinfo/xen-devel
^ permalink raw reply
* [PATCH 0/6] Ethernet over hdlc
From: David Gounaris @ 2018-08-28 11:09 UTC (permalink / raw)
To: qiang.zhao, netdev, linuxppc-dev; +Cc: David Gounaris
Hello!
I have a 83xx platform and I am using non tsa mode in the fsl hdlc driver to enable ethernet over hdlc. These patches are the minimal changes that I needed to do in order to get it to work with our legacy products that used old FSL SDK driver. Please review.
Best Regards,
David Gounaris
David Gounaris (6):
net/wan/fsl_ucc_hdlc: allow ucc index up to 4
net/wan/fsl_ucc_hdlc: allow PARITY_CRC16_PR0_CCITT parity
net/wan/fsl_ucc_hdlc: Adding ARPHRD_ETHER
net/wan/fsl_ucc_hdlc: default hmask value
net/wan/fsl_ucc_hdlc: GUMR for non tsa mode
net/wan/fsl_ucc_hdlc: tx timeout handler
drivers/net/wan/fsl_ucc_hdlc.c | 23 +++++++++++++++++++++--
drivers/net/wan/fsl_ucc_hdlc.h | 2 +-
2 files changed, 22 insertions(+), 3 deletions(-)
--
2.13.6
^ permalink raw reply
* [PATCH 4/6] net/wan/fsl_ucc_hdlc: default hmask value
From: David Gounaris @ 2018-08-28 11:09 UTC (permalink / raw)
To: qiang.zhao, netdev, linuxppc-dev; +Cc: David Gounaris
In-Reply-To: <20180828110921.2542-1-david.gounaris@infinera.com>
Set default HMASK to 0x0000 to use
promiscuous mode in the hdlc controller.
Signed-off-by: David Gounaris <david.gounaris@infinera.com>
---
drivers/net/wan/fsl_ucc_hdlc.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.h b/drivers/net/wan/fsl_ucc_hdlc.h
index c21134c1f180..5bc3d1a6ca6e 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.h
+++ b/drivers/net/wan/fsl_ucc_hdlc.h
@@ -134,7 +134,7 @@ struct ucc_hdlc_private {
#define HDLC_HEAD_MASK 0x0000
#define DEFAULT_HDLC_HEAD 0xff44
-#define DEFAULT_ADDR_MASK 0x00ff
+#define DEFAULT_ADDR_MASK 0x0000
#define DEFAULT_HDLC_ADDR 0x00ff
#define BMR_GBL 0x20000000
--
2.13.6
^ permalink raw reply related
* [PATCH 5/6] net/wan/fsl_ucc_hdlc: GUMR for non tsa mode
From: David Gounaris @ 2018-08-28 11:09 UTC (permalink / raw)
To: qiang.zhao, netdev, linuxppc-dev; +Cc: David Gounaris
In-Reply-To: <20180828110921.2542-1-david.gounaris@infinera.com>
The following bits in the GUMR is changed for non
tsa mode: CDS, CTSP and CTSS are set to zero.
When set, there is no tx interrupts from the controller.
Signed-off-by: David Gounaris <david.gounaris@infinera.com>
---
drivers/net/wan/fsl_ucc_hdlc.c | 7 +++++++
1 file changed, 7 insertions(+)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 54e2b2143e36..e6154a6547e6 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -97,6 +97,13 @@ static int uhdlc_init(struct ucc_hdlc_private *priv)
if (priv->tsa) {
uf_info->tsa = 1;
uf_info->ctsp = 1;
+ uf_info->cds = 1;
+ uf_info->ctss = 1;
+ }
+ else {
+ uf_info->cds = 0;
+ uf_info->ctsp = 0;
+ uf_info->ctss = 0;
}
/* This sets HPM register in CMXUCR register which configures a
--
2.13.6
^ permalink raw reply related
* [PATCH 3/6] net/wan/fsl_ucc_hdlc: Adding ARPHRD_ETHER
From: David Gounaris @ 2018-08-28 11:09 UTC (permalink / raw)
To: qiang.zhao, netdev, linuxppc-dev; +Cc: David Gounaris
In-Reply-To: <20180828110921.2542-1-david.gounaris@infinera.com>
This was done to avoid discarding ethernet
packets when using HDLC_ETH protocol.
Signed-off-by: David Gounaris <david.gounaris@infinera.com>
---
drivers/net/wan/fsl_ucc_hdlc.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 963b3b5ae15e..54e2b2143e36 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -375,6 +375,10 @@ static netdev_tx_t ucc_hdlc_tx(struct sk_buff *skb, struct net_device *dev)
dev->stats.tx_bytes += skb->len;
break;
+ case ARPHRD_ETHER:
+ dev->stats.tx_bytes += skb->len;
+ break;
+
default:
dev->stats.tx_dropped++;
dev_kfree_skb(skb);
@@ -512,6 +516,8 @@ static int hdlc_rx_done(struct ucc_hdlc_private *priv, int rx_work_limit)
break;
case ARPHRD_PPP:
+ case ARPHRD_ETHER:
+
length -= HDLC_CRC_SIZE;
skb = dev_alloc_skb(length);
--
2.13.6
^ permalink raw reply related
* [PATCH 1/6] net/wan/fsl_ucc_hdlc: allow ucc index up to 4
From: David Gounaris @ 2018-08-28 11:09 UTC (permalink / raw)
To: qiang.zhao, netdev, linuxppc-dev; +Cc: David Gounaris
In-Reply-To: <20180828110921.2542-1-david.gounaris@infinera.com>
There is a need to allow higher indexes to be
able to support MPC83xx platforms. (UCC1-UCC5)
Signed-off-by: David Gounaris <david.gounaris@infinera.com>
---
drivers/net/wan/fsl_ucc_hdlc.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wan/fsl_ucc_hdlc.c b/drivers/net/wan/fsl_ucc_hdlc.c
index 5f0366a125e2..3c0e0a1d19ba 100644
--- a/drivers/net/wan/fsl_ucc_hdlc.c
+++ b/drivers/net/wan/fsl_ucc_hdlc.c
@@ -1015,7 +1015,7 @@ static int ucc_hdlc_probe(struct platform_device *pdev)
}
ucc_num = val - 1;
- if ((ucc_num > 3) || (ucc_num < 0)) {
+ if ((ucc_num > 4) || (ucc_num < 0)) {
dev_err(&pdev->dev, ": Invalid UCC num\n");
return -EINVAL;
}
--
2.13.6
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox