lkml.org 
[lkml]   [2020]   [Sep]   [29]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
SubjectRe: [patch V2 11/36] net: ionic: Replace in_interrupt() usage.
From
Date
On 9/29/20 1:25 PM, Thomas Gleixner wrote:
> From: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
>
> The in_interrupt() usage in this driver tries to figure out which context
> may sleep and which context may not sleep. in_interrupt() is not really
> suitable as it misses both preemption disabled and interrupt disabled
> invocations from task context.
>
> Conditionals like that in driver code are frowned upon in general because
> invocations of functions from invalid contexts might not be detected
> as the conditional papers over it.
>
> ionic_lif_addr() and _ionoc_lif_rx_mode() can be called from:
>
> 1) ->ndo_set_rx_mode() which is under netif_addr_lock_bh()) so it must not
> sleep.
>
> 2) Init and setup functions which are in fully preemptible task context.
>
> ionic_link_status_check_request() has two call paths:
>
> 1) NAPI which obviously cannot sleep
>
> 2) Setup which is again fully preemptible task context
>
> Add arguments which convey the execution context to the affected functions
> and let the callers provide the context instead of letting the functions
> deduce it.
>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> ---
> V2: Treat _ionoc_lif_rx_mode() correclty (Shannon)

Is it fair to poke at the comments?
s/ionoc/ionic/
s/correclty/correctly/

> ---
> drivers/net/ethernet/pensando/ionic/ionic_dev.c | 2
> drivers/net/ethernet/pensando/ionic/ionic_lif.c | 64 ++++++++++++++++--------
> drivers/net/ethernet/pensando/ionic/ionic_lif.h | 2
> 3 files changed, 47 insertions(+), 21 deletions(-)
>
> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
> @@ -22,7 +22,7 @@ static void ionic_watchdog_cb(struct tim
> hb = ionic_heartbeat_check(ionic);
>
> if (hb >= 0 && ionic->lif)
> - ionic_link_status_check_request(ionic->lif);
> + ionic_link_status_check_request(ionic->lif, false);
> }
>
> void ionic_init_devinfo(struct ionic *ionic)
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.c
> @@ -151,7 +151,7 @@ static void ionic_link_status_check(stru
> clear_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state);
> }
>
> -void ionic_link_status_check_request(struct ionic_lif *lif)
> +void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep)
> {
> struct ionic_deferred_work *work;
>
> @@ -159,7 +159,7 @@ void ionic_link_status_check_request(str
> if (test_and_set_bit(IONIC_LIF_F_LINK_CHECK_REQUESTED, lif->state))
> return;
>
> - if (in_interrupt()) {
> + if (!can_sleep) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work)
> return;
> @@ -798,7 +798,7 @@ static bool ionic_notifyq_service(struct
>
> switch (le16_to_cpu(comp->event.ecode)) {
> case IONIC_EVENT_LINK_CHANGE:
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, false);
> break;
> case IONIC_EVENT_RESET:
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> @@ -981,7 +981,8 @@ static int ionic_lif_addr_del(struct ion
> return 0;
> }
>
> -static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add)
> +static int ionic_lif_addr(struct ionic_lif *lif, const u8 *addr, bool add,
> + bool can_sleep)
> {
> struct ionic *ionic = lif->ionic;
> struct ionic_deferred_work *work;
> @@ -1010,7 +1011,7 @@ static int ionic_lif_addr(struct ionic_l
> lif->nucast--;
> }
>
> - if (in_interrupt()) {
> + if (!can_sleep) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work) {
> netdev_err(lif->netdev, "%s OOM\n", __func__);
> @@ -1036,12 +1037,22 @@ static int ionic_lif_addr(struct ionic_l
>
> static int ionic_addr_add(struct net_device *netdev, const u8 *addr)
> {
> - return ionic_lif_addr(netdev_priv(netdev), addr, true);
> + return ionic_lif_addr(netdev_priv(netdev), addr, true, true);
> +}
> +
> +static int ionic_ndo_addr_add(struct net_device *netdev, const u8 *addr)
> +{
> + return ionic_lif_addr(netdev_priv(netdev), addr, true, false);
> }
>
> static int ionic_addr_del(struct net_device *netdev, const u8 *addr)
> {
> - return ionic_lif_addr(netdev_priv(netdev), addr, false);
> + return ionic_lif_addr(netdev_priv(netdev), addr, false, true);
> +}
> +
> +static int ionic_ndo_addr_del(struct net_device *netdev, const u8 *addr)
> +{
> + return ionic_lif_addr(netdev_priv(netdev), addr, false, false);
> }

These changes are reasonable, tho' I'm not fond of parameter lists of
"true" and "false" with no context.  I'd prefer to have some constants like
#define can_sleep true
so the code can be a little more readable.

Yes, I know we have this problem already in the call to
ionic_lif_addr(), which I'm annoyed with but haven't addressed yet.

So, if you want to deal with this now, fine, otherwise I'll take care of
them both later.

Meanwhile,

Acked-by: Shannon Nelson <snelson@pensando.io>




>
> static void ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
> @@ -1081,11 +1092,12 @@ static void ionic_lif_rx_mode(struct ion
> lif->rx_mode = rx_mode;
> }
>
> -static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode)
> +static void _ionic_lif_rx_mode(struct ionic_lif *lif, unsigned int rx_mode,
> + bool from_ndo)
> {
> struct ionic_deferred_work *work;
>
> - if (in_interrupt()) {
> + if (from_ndo) {
> work = kzalloc(sizeof(*work), GFP_ATOMIC);
> if (!work) {
> netdev_err(lif->netdev, "%s OOM\n", __func__);
> @@ -1100,7 +1112,16 @@ static void _ionic_lif_rx_mode(struct io
> }
> }
>
> -static void ionic_set_rx_mode(struct net_device *netdev)
> +static void ionic_dev_uc_sync(struct net_device *netdev, bool from_ndo)
> +{
> + if (from_ndo)
> + __dev_uc_sync(netdev, ionic_ndo_addr_add, ionic_ndo_addr_del);
> + else
> + __dev_uc_sync(netdev, ionic_addr_add, ionic_addr_del);
> +
> +}
> +
> +static void ionic_set_rx_mode(struct net_device *netdev, bool from_ndo)
> {
> struct ionic_lif *lif = netdev_priv(netdev);
> struct ionic_identity *ident;
> @@ -1122,7 +1143,7 @@ static void ionic_set_rx_mode(struct net
> * we remove our overflow flag and check the netdev flags
> * to see if we can disable NIC PROMISC
> */
> - __dev_uc_sync(netdev, ionic_addr_add, ionic_addr_del);
> + ionic_dev_uc_sync(netdev, from_ndo);
> nfilters = le32_to_cpu(ident->lif.eth.max_ucast_filters);
> if (netdev_uc_count(netdev) + 1 > nfilters) {
> rx_mode |= IONIC_RX_MODE_F_PROMISC;
> @@ -1134,7 +1155,7 @@ static void ionic_set_rx_mode(struct net
> }
>
> /* same for multicast */
> - __dev_mc_sync(netdev, ionic_addr_add, ionic_addr_del);
> + ionic_dev_uc_sync(netdev, from_ndo);
> nfilters = le32_to_cpu(ident->lif.eth.max_mcast_filters);
> if (netdev_mc_count(netdev) > nfilters) {
> rx_mode |= IONIC_RX_MODE_F_ALLMULTI;
> @@ -1146,7 +1167,12 @@ static void ionic_set_rx_mode(struct net
> }
>
> if (lif->rx_mode != rx_mode)
> - _ionic_lif_rx_mode(lif, rx_mode);
> + _ionic_lif_rx_mode(lif, rx_mode, from_ndo);
> +}
> +
> +static void ionic_ndo_set_rx_mode(struct net_device *netdev)
> +{
> + ionic_set_rx_mode(netdev, true);
> }
>
> static __le64 ionic_netdev_features_to_nic(netdev_features_t features)
> @@ -1391,7 +1417,7 @@ static int ionic_start_queues_reconfig(s
> */
> err = ionic_txrx_init(lif);
> mutex_unlock(&lif->queue_lock);
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, true);
> netif_device_attach(lif->netdev);
>
> return err;
> @@ -1720,7 +1746,7 @@ static int ionic_txrx_init(struct ionic_
> if (lif->netdev->features & NETIF_F_RXHASH)
> ionic_lif_rss_init(lif);
>
> - ionic_set_rx_mode(lif->netdev);
> + ionic_set_rx_mode(lif->netdev, false);
>
> return 0;
>
> @@ -2093,7 +2119,7 @@ static const struct net_device_ops ionic
> .ndo_stop = ionic_stop,
> .ndo_start_xmit = ionic_start_xmit,
> .ndo_get_stats64 = ionic_get_stats64,
> - .ndo_set_rx_mode = ionic_set_rx_mode,
> + .ndo_set_rx_mode = ionic_ndo_set_rx_mode,
> .ndo_set_features = ionic_set_features,
> .ndo_set_mac_address = ionic_set_mac_address,
> .ndo_validate_addr = eth_validate_addr,
> @@ -2521,7 +2547,7 @@ static void ionic_lif_handle_fw_up(struc
> }
>
> clear_bit(IONIC_LIF_F_FW_RESET, lif->state);
> - ionic_link_status_check_request(lif);
> + ionic_link_status_check_request(lif, true);
> netif_device_attach(lif->netdev);
> dev_info(ionic->dev, "FW Up: LIFs restarted\n");
>
> @@ -2713,7 +2739,7 @@ static int ionic_station_set(struct ioni
> */
> if (!ether_addr_equal(ctx.comp.lif_getattr.mac,
> netdev->dev_addr))
> - ionic_lif_addr(lif, netdev->dev_addr, true);
> + ionic_lif_addr(lif, netdev->dev_addr, true, true);
> } else {
> /* Update the netdev mac with the device's mac */
> memcpy(addr.sa_data, ctx.comp.lif_getattr.mac, netdev->addr_len);
> @@ -2730,7 +2756,7 @@ static int ionic_station_set(struct ioni
>
> netdev_dbg(lif->netdev, "adding station MAC addr %pM\n",
> netdev->dev_addr);
> - ionic_lif_addr(lif, netdev->dev_addr, true);
> + ionic_lif_addr(lif, netdev->dev_addr, true, true);
>
> return 0;
> }
> --- a/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> +++ b/drivers/net/ethernet/pensando/ionic/ionic_lif.h
> @@ -245,7 +245,7 @@ static inline u32 ionic_coal_usec_to_hw(
>
> typedef void (*ionic_reset_cb)(struct ionic_lif *lif, void *arg);
>
> -void ionic_link_status_check_request(struct ionic_lif *lif);
> +void ionic_link_status_check_request(struct ionic_lif *lif, bool can_sleep);
> void ionic_get_stats64(struct net_device *netdev,
> struct rtnl_link_stats64 *ns);
> void ionic_lif_deferred_enqueue(struct ionic_deferred *def,
>

\
 
 \ /
  Last update: 2020-09-30 01:07    [W:0.139 / U:0.404 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site