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=-2.4 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_HELO_NONE, SPF_PASS,USER_AGENT_SANE_1 autolearn=no 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 5429FC2D0BF for ; Tue, 17 Dec 2019 21:00:38 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1362F21582 for ; Tue, 17 Dec 2019 21:00:38 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=ziepe.ca header.i=@ziepe.ca header.b="cuzK5VjP" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728011AbfLQVAh (ORCPT ); Tue, 17 Dec 2019 16:00:37 -0500 Received: from mail-qv1-f66.google.com ([209.85.219.66]:33841 "EHLO mail-qv1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1726996AbfLQVAh (ORCPT ); Tue, 17 Dec 2019 16:00:37 -0500 Received: by mail-qv1-f66.google.com with SMTP id o18so4767224qvf.1 for ; Tue, 17 Dec 2019 13:00:36 -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=IvjWMA+q5S2q7e5kFQNGF//nvUxKMp0Z+m3ILb72NO8=; b=cuzK5VjPeMRhneOuXGqHYN1IFcfrPZkl5N9TW4FjMXLsWQbnSb2ldeYEO/4YhuSjDy 6DOq0BWzGfaQC5N5MrKjJ0RanG5NaqsGLOQik4aBWIjNRe62VZTu0AyutN3kBSgVBVUF Akkk7j5sFQ1W+Hjwdn78CFNYs+vYCGMoCTVc5yGXVY3MuhcTYgNUgSt10L6s1JkXjI7X /dFR9w52J2fuNbZF2Y70mfrfnZGwakm3gtGgyz1vphB6NS4TlbWLCQPII5BjzumxtlTW N2/l4XuAS5YYOCBNURueWzyNFDZOCeMVAx2p0bNtPogUko5ezqcPYtTyxXnWZhx9f0v3 ZziA== 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=IvjWMA+q5S2q7e5kFQNGF//nvUxKMp0Z+m3ILb72NO8=; b=jYeMg1WYJeV66tUxQ3oFXp3H/iSzDATGaBpN9x//E/ZIIVX8uBnPY3v03vyjbVtQff TMNDbcLotNFVzFeTkSNwGkjrL4fggzVDOH3LaDNDrXqZcLJiBNZ/F+rd56iwYdcl4Laj XJgNpM+Ay9396llrFCnsJVYdqWEMzMAwN/mYjQOqxif1UCO4B5RwtKZT9mk5yRp8FVRN AqHbiS1z4yIvUlxNFtnaNtPwqsF66S62Ubvfi3qvNK52Vibib5cVx0ao8gZHYS+sflfP aLM8OiZKeNk9cq4CSeAqS1DHzAzHTUyIhYNz3wcMujvbedBiaiO8sahPZCCFZ1N0u++G 9tZQ== X-Gm-Message-State: APjAAAXOb1VVjaADox+uG9E4+fwPiU9CjC3v1IgN7YVg98/9yYLD0W8Z WzC696uNgiwSztla9soCuQARNg== X-Google-Smtp-Source: APXvYqxk8JLLr0ZCkGikt79tFYLoUje06GWJ/dg8jfHCmSAWS6QfmEmI6BkmmTD0GBhO+RBqD1xryw== X-Received: by 2002:ad4:4021:: with SMTP id q1mr6352088qvp.211.1576616435490; Tue, 17 Dec 2019 13:00:35 -0800 (PST) Received: from ziepe.ca (hlfxns017vw-142-68-57-212.dhcp-dynamic.fibreop.ns.bellaliant.net. [142.68.57.212]) by smtp.gmail.com with ESMTPSA id t7sm7423135qkm.136.2019.12.17.13.00.34 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Tue, 17 Dec 2019 13:00:34 -0800 (PST) Received: from jgg by mlx.ziepe.ca with local (Exim 4.90_1) (envelope-from ) id 1ihJxJ-0002GJ-W8; Tue, 17 Dec 2019 17:00:34 -0400 Date: Tue, 17 Dec 2019 17:00:33 -0400 From: Jason Gunthorpe To: "Saleem, Shiraz" Cc: "Kirsher, Jeffrey T" , "davem@davemloft.net" , "gregkh@linuxfoundation.org" , "Ismail, Mustafa" , "netdev@vger.kernel.org" , "linux-rdma@vger.kernel.org" , "nhorman@redhat.com" , "sassmann@redhat.com" , "parav@mellanox.com" Subject: Re: [PATCH v3 05/20] RDMA/irdma: Add driver framework definitions Message-ID: <20191217210033.GB17227@ziepe.ca> References: <20191209224935.1780117-1-jeffrey.t.kirsher@intel.com> <20191209224935.1780117-6-jeffrey.t.kirsher@intel.com> <20191210190438.GF46@ziepe.ca> <9DD61F30A802C4429A01CA4200E302A7B6B8FBCA@fmsmsx124.amr.corp.intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <9DD61F30A802C4429A01CA4200E302A7B6B8FBCA@fmsmsx124.amr.corp.intel.com> User-Agent: Mutt/1.9.4 (2018-02-28) Sender: linux-rdma-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-rdma@vger.kernel.org On Thu, Dec 12, 2019 at 01:40:27AM +0000, Saleem, Shiraz wrote: > > > +/* client interface functions */ > > > +static const struct i40e_client_ops i40e_ops = { > > > + .open = i40iw_open, > > > + .close = i40iw_close, > > > + .l2_param_change = i40iw_l2param_change }; > > > > Wasn't the whole point of virtual bus to avoid stuff like this? Why isn't a client the > > virtual bus object and this information extended into the driver ops? > > These are the private interface calls between lan and rdma. > These ops are implemented by RDMA driver but invoked by > netdev driver. AFAIK you are supposed to provide things like this as part of your device driver ops, ie open is probe, close is unbind, etc. > > > +/** > > > + * irdma_event_handler - Called by LAN driver to notify events > > > + * @ldev: Peer device structure > > > + * @event: event from LAN driver > > > + */ > > > +static void irdma_event_handler(struct iidc_peer_dev *ldev, > > > + struct iidc_event *event) > > > +{ > > > + struct irdma_l2params l2params = {}; > > > + struct irdma_device *iwdev; > > > + int i; > > > + > > > + iwdev = irdma_get_device(ldev->netdev); > > > + if (!iwdev) > > > + return; > > > + > > > + if (test_bit(IIDC_EVENT_LINK_CHANGE, event->type)) { > > > > Is this atomic? Why using test_bit? > No its not. What do you suggest we use? Just test the bit? if (event->type & BIT(IIDC_EVENT_LINK_CHANGE)) ? test_bit is the atomic version of those that matches atomic set_bit/clear_bit > > > + ldev->ops->reg_for_notification(ldev, &events); > > > + dev_info(rfdev_to_dev(dev), "IRDMA VSI Open Successful"); > > > > Lets not do this kind of logging.. > > > > There is some dev_info which should be cleaned up to dev_dbg. > But logging this info is useful to know that this functions VSI (and associated ibdev) > is up and reading for RDMA traffic. > Is info logging to be avoided altogether? Certainly not at display-by-default level. If every driver printed when it binds we'd have a mess. > > > new file mode 100644 > > > index 000000000000..b418e76a3302 > > > +++ b/drivers/infiniband/hw/irdma/main.c > > > @@ -0,0 +1,630 @@ > > > +// SPDX-License-Identifier: GPL-2.0 or Linux-OpenIB > > > +/* Copyright (c) 2015 - 2019 Intel Corporation */ #include "main.h" > > > + > > > +/* Legacy i40iw module parameters */ > > > +static int resource_profile; > > > +module_param(resource_profile, int, 0644); > > > +MODULE_PARM_DESC(resource_profile, "Resource Profile: 0=PF only, > > > +1=Weighted VF, 2=Even Distribution"); > > > + > > > +static int max_rdma_vfs = 32; > > > +module_param(max_rdma_vfs, int, 0644); > > MODULE_PARM_DESC(max_rdma_vfs, > > > +"Maximum VF count: 0-32 32=default"); > > > + > > > +static int mpa_version = 2; > > > +module_param(mpa_version, int, 0644); MODULE_PARM_DESC(mpa_version, > > > +"MPA version: deprecated parameter"); > > > + > > > +static int push_mode; > > > +module_param(push_mode, int, 0644); > > > +MODULE_PARM_DESC(push_mode, "Low latency mode: deprecated > > > +parameter"); > > > + > > > +static int debug; > > > +module_param(debug, int, 0644); > > > +MODULE_PARM_DESC(debug, "debug flags: deprecated parameter"); > > > > Generally no to module parameters > > Agree. But these are module params that existed in i40iw. > And irdma replaces i40iw and has a module alias > for it. Provide non-module parameter alternatives for all of these, and it can be OK only because of the module alias, and only if Doug lets you remove i40iw. > > > +static struct workqueue_struct *irdma_wq; > > > > Another wq already? > This wq is used for deferred handling of irdma service tasks. > Such as rebuild/recovery after reset. We have system global wqs. Why are they not OK? Jason