From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-8.6 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SIGNED_OFF_BY,SPF_PASS,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 42657C43381 for ; Fri, 15 Feb 2019 17:47:19 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 03BB3222BE for ; Fri, 15 Feb 2019 17:47:18 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b="Zfv8/GIV" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1729064AbfBORrR (ORCPT ); Fri, 15 Feb 2019 12:47:17 -0500 Received: from mail-pf1-f193.google.com ([209.85.210.193]:37416 "EHLO mail-pf1-f193.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726819AbfBORrR (ORCPT ); Fri, 15 Feb 2019 12:47:17 -0500 Received: by mail-pf1-f193.google.com with SMTP id s22so5167624pfh.4 for ; Fri, 15 Feb 2019 09:47:16 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=ziepe.ca; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=qZFJIABHAHRLhqwPCcJzyA3qvWX9h8jR+Q/1io6APGs=; b=Zfv8/GIV+qn0urg8XaRzcv5gYPzSRO1gtvBzmcG9IfSOR2QezR1qAPcoWw8EV5JK29 wd20+1r2a0gKEzw+zu56eb7kiI4MvhdVTrujshsRQtuRfZBsCxsl6X5cziI496yy98en nih0ZBncqKY3jju5CyoRlt+/wI9k/Bz/cW6DNR+FyHglnAURW1Z9HtIObJ/hF7q6u704 Tt/n9A5Jtb8VVWCeRmkJeCb2PabcUNEsEQUCya1CUPVK3qwPS/BVokWP30wDI9zvXLpC hUATynAa+rKbrjTFlK7RYxaWN4YcnZUD9U8EA2ut9YhN2nJSOab2PX2bWYxorZsOXTlq rwWQ== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=qZFJIABHAHRLhqwPCcJzyA3qvWX9h8jR+Q/1io6APGs=; b=CZJ5cX+8NM2IjLGHyAoW3v7sNuOJj021Z8hPAc7S/b2kDSGFdiCMVBOGnM2ExF2+44 g7erorPVLmUh5HnsG5Sw+TcYiAQ3fPNAoOL3Z+H1MCSp3mCPyw6xFQL89NHRgFHjSeIO +T4lHcKMhfZmUl+7QjJ2WTtgFBhE64Rkxwuh/1nwvYinDpglsawLcwVbIz1s4Sb93idp QszluvSFnkjkRSooyHn7CGFoeyqZwxjjWCnP4i1gjKmxjIS+slk1vP7r4W8t0oKjN8Gw sozNUPIVG3WxLOMLgU4vX5sDyhUgCTvB30yN3Ph88h0481swQ5qSbszNxKTh3XdBK7Rc 7vKA== X-Gm-Message-State: AHQUAuYREyUCzy0Y5Z84NWa+t3vxomrSY6skghqQkDC+LDorSxvTVH6f EVoEQ4y+q1GgYCJeFbH3NzeHbQ== X-Google-Smtp-Source: AHgI3IZIxIB9bz6kvaxbtNEPaPNpvS3bd0IpGKUirfgE0CTRKuhmCwvxwx1ttZbOli4YivE0xC3liA== X-Received: by 2002:a62:4d81:: with SMTP id a123mr11224375pfb.122.1550252836135; Fri, 15 Feb 2019 09:47:16 -0800 (PST) Received: from ziepe.ca (S010614cc2056d97f.ed.shawcable.net. [174.3.196.123]) by smtp.gmail.com with ESMTPSA id n27sm12046715pfb.8.2019.02.15.09.47.15 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Fri, 15 Feb 2019 09:47:15 -0800 (PST) Received: from jgg by mlx.ziepe.ca with local (Exim 4.90_1) (envelope-from ) id 1guhZy-0000Yv-QK; Fri, 15 Feb 2019 10:47:14 -0700 Date: Fri, 15 Feb 2019 10:47:14 -0700 From: Jason Gunthorpe To: Shiraz Saleem Cc: dledford@redhat.com, davem@davemloft.net, linux-rdma@vger.kernel.org, netdev@vger.kernel.org, mustafa.ismail@intel.com, jeffrey.t.kirsher@intel.com Subject: Re: [RFC v1 15/19] RDMA/irdma: Add miscellaneous utility definitions Message-ID: <20190215174714.GE30706@ziepe.ca> References: <20190215171107.6464-1-shiraz.saleem@intel.com> <20190215171107.6464-16-shiraz.saleem@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20190215171107.6464-16-shiraz.saleem@intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: netdev-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: netdev@vger.kernel.org On Fri, Feb 15, 2019 at 11:11:02AM -0600, Shiraz Saleem wrote: > From: Mustafa Ismail > > Add miscellaneous utility functions and headers. > > Signed-off-by: Mustafa Ismail > Signed-off-by: Shiraz Saleem > drivers/infiniband/hw/irdma/osdep.h | 153 ++ > drivers/infiniband/hw/irdma/protos.h | 118 ++ > drivers/infiniband/hw/irdma/status.h | 70 + > drivers/infiniband/hw/irdma/utils.c | 2565 ++++++++++++++++++++++++++++++++++ > 4 files changed, 2906 insertions(+) > create mode 100644 drivers/infiniband/hw/irdma/osdep.h > create mode 100644 drivers/infiniband/hw/irdma/protos.h > create mode 100644 drivers/infiniband/hw/irdma/status.h > create mode 100644 drivers/infiniband/hw/irdma/utils.c > > diff --git a/drivers/infiniband/hw/irdma/osdep.h b/drivers/infiniband/hw/irdma/osdep.h > new file mode 100644 > index 0000000..ade5536 > +++ b/drivers/infiniband/hw/irdma/osdep.h > @@ -0,0 +1,153 @@ > +/* SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB */ > +/* Copyright (c) 2019, Intel Corporation. */ > + > +#ifndef IRDMA_OSDEP_H > +#define IRDMA_OSDEP_H > + > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +#include > +/* get readq/writeq support for 32 bit kernels, use the low-first version */ > +#include > + > +#define STATS_TIMER_DELAY 60000 > +#define MAKEMASK(m, s) ((m) << (s)) > + > +#define irdma_pr_err(fmt, args ...) \ > + pr_err("%s: "fmt, __func__, ## args) > + > +#define irdma_pr_info(fmt, args ...) \ > + pr_info("%s: " fmt, __func__, ## args) > + > +#define irdma_pr_warn(fmt, args ...) \ > + pr_warn("%s: " fmt, __func__, ## args) > + > +#define irdma_dev_err(dev, fmt, args ...) \ > + dev_err(to_device(dev), "%s: "fmt, __func__, ## args) > + > +#define irdma_dev_info(dev, fmt, args ...) \ > + dev_info(to_device(dev), "%s: "fmt, __func__, ## args) > + > +#define irdma_dev_warn(dev, fmt, args ...) \ > + dev_warn(to_device(dev), "%s: "fmt, __func__, ## args) Does every driver really have to define these macros? > +#define to_device(ptr) \ > + (&((struct pci_dev *)((ptr)->hw->dev_context))->dev) ?? Seems like this wants to be container_of?? > +/** > + * irdma_insert_wqe_hdr - write wqe header > + * @wqe: cqp wqe for header > + * @header: header for the cqp wqe > + */ > +static inline void irdma_insert_wqe_hdr(__le64 *wqe, u64 hdr) > +{ > + wmb(); /* make sure WQE is populated before polarity is set */ > + set_64bit_val(wqe, 24, hdr); Generally don't like seeing wmbs in drivers.. Are you sure this isn't supposed to be smp_store_release(), or dma_wmb() perhaps? > +/** > + * irdma_inetaddr_event - system notifier for ipv4 addr events > + * @notfier: not used > + * @event: event for notifier > + * @ptr: if address > + */ > +int irdma_inetaddr_event(struct notifier_block *notifier, > + unsigned long event, > + void *ptr) > +{ > + struct in_ifaddr *ifa = ptr; > + struct net_device *event_netdev = ifa->ifa_dev->dev; > + struct net_device *netdev; > + struct net_device *upper_dev; > + struct irdma_device *iwdev; > + u32 local_ipaddr; > + > + iwdev = irdma_find_netdev(event_netdev); This is all being changed too (and is probably wrongly locked here) A new driver must not maintain their own list of devices. > + if (iwdev->init_state < IP_ADDR_REGISTERED || iwdev->closing) > + return NOTIFY_DONE; > + > + netdev = iwdev->netdev; > + upper_dev = netdev_master_upper_dev_get(netdev); > + if (netdev != event_netdev) > + return NOTIFY_DONE; What is all this? Does the driver support bonding? You have to fix this to work in the new style - and you might need to add more core code to sanely support bonding. Look here: https://patchwork.kernel.org/project/linux-rdma/list/?series=79299 > +/** > + * irdma_add_devusecount - add dev refcount > + * @iwdev: dev for refcount > + */ > +void irdma_add_devusecount(struct irdma_device *iwdev) > +{ > + atomic64_inc(&iwdev->use_count); > +} > + > +/** > + * irdma_rem_devusecount - decrement refcount for dev > + * @iwdev: device > + */ > +void irdma_rem_devusecount(struct irdma_device *iwdev) > +{ > + if (!atomic64_dec_and_test(&iwdev->use_count)) > + return; > + wake_up(&iwdev->close_wq); > +} > + > +/** > + * irdma_add_pdusecount - add pd refcount > + * @iwpd: pd for refcount > + */ > +void irdma_add_pdusecount(struct irdma_pd *iwpd) > +{ > + atomic_inc(&iwpd->usecount); > +} Why do we have these wrappers? Don't like wrappers liket his. Are you sure this should be an atomic, not a kref, refcount, etc? Very concerning to refcounting of HW object structures like this.. Most often when I see this in IB drivers it comes along with concurrency bugs in the destroy path. > +/** > + * irdma_allocate_dma_mem - Memory alloc helper fn > + * @hw: pointer to the HW structure > + * @mem: ptr to mem struct to fill out > + * @size: size of memory requested > + * @alignment: what to align the allocation to > + */ > +enum irdma_status_code irdma_allocate_dma_mem(struct irdma_hw *hw, > + struct irdma_dma_mem *mem, > + u64 size, > + u32 alignment) > +{ > + struct pci_dev *pcidev = (struct pci_dev *)hw->dev_context; > + > + if (!mem) > + return IRDMA_ERR_PARAM; > + > + mem->size = ALIGN(size, alignment); > + mem->va = dma_alloc_coherent(&pcidev->dev, mem->size, > + (dma_addr_t *)&mem->pa, GFP_KERNEL); > + if (!mem->va) > + return IRDMA_ERR_NO_MEMORY; > + > + return 0; > +} More wrappers? Why? Jason