From: Jason Gunthorpe <jgg@ziepe•ca>
To: Jeff Kirsher <jeffrey.t.kirsher@intel•com>
Cc: davem@davemloft•net, gregkh@linuxfoundation•org,
Mustafa Ismail <mustafa.ismail@intel•com>,
netdev@vger•kernel.org, linux-rdma@vger•kernel.org,
nhorman@redhat•com, sassmann@redhat•com, parav@mellanox•com,
Shiraz Saleem <shiraz.saleem@intel•com>
Subject: Re: [PATCH v3 05/20] RDMA/irdma: Add driver framework definitions
Date: Tue, 10 Dec 2019 15:04:38 -0400 [thread overview]
Message-ID: <20191210190438.GF46@ziepe.ca> (raw)
In-Reply-To: <20191209224935.1780117-6-jeffrey.t.kirsher@intel.com>
On Mon, Dec 09, 2019 at 02:49:20PM -0800, Jeff Kirsher wrote:
> +{
> + struct i40e_info *ldev = (struct i40e_info *)rf->ldev.if_ldev;
Why are there so many casts in this file? Is this really container of?
> + hdl = kzalloc((sizeof(*hdl) + sizeof(*iwdev)), GFP_KERNEL);
> + if (!hdl)
> + return -ENOMEM;
> +
> + iwdev = (struct irdma_device *)((u8 *)hdl + sizeof(*hdl));
Yikes, use structs and container of for things like this please.
> + iwdev->param_wq = alloc_ordered_workqueue("l2params", WQ_MEM_RECLAIM);
> + if (!iwdev->param_wq)
> + goto error;
Leon usually asks why another work queue at this point, at least have
a comment justifying why. Shouldn't it have a better name?
> +/* 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?
> +int i40iw_probe(struct virtbus_device *vdev)
> +{
> + struct i40e_info *ldev =
> + container_of(vdev, struct i40e_info, vdev);
> +
> + if (!ldev)
> + return -EINVAL;
eh? how can that happen
> +
> + if (!ldev->ops->client_device_register)
> + return -EINVAL;
How can this happen too? If it doesn't support register then don't
create a virtual device, surely?
I've really developed a strong distate to these random non-functional
'ifs' that seem to get into things.
If it is functional then fine, but if it is an assertion write it as
if (WARN_ON()) to make it clear to readers it can't happen by design
> +/**
> + * irdma_lan_register_qset - Register qset with LAN driver
> + * @vsi: vsi structure
> + * @tc_node: Traffic class node
> + */
> +static enum irdma_status_code irdma_lan_register_qset(struct irdma_sc_vsi *vsi,
> + struct irdma_ws_node *tc_node)
> +{
> + struct irdma_device *iwdev = vsi->back_vsi;
> + struct iidc_peer_dev *ldev = (struct iidc_peer_dev *)iwdev->ldev->if_ldev;
Again with the casts.. Please try to clean up the casting in this driver
> + struct iidc_res rdma_qset_res = {};
> + int ret;
> +
> + if (ldev->ops->alloc_res) {
Quite an abnormal coding style to put the entire function under an if,
just if() return 0 ? Many examples of this
> +/**
> + * irdma_log_invalid_mtu: log warning on invalid mtu
> + * @mtu: maximum tranmission unit
> + */
> +static void irdma_log_invalid_mtu(u16 mtu)
> +{
> + if (mtu < IRDMA_MIN_MTU_IPV4)
> + pr_warn("Current MTU setting of %d is too low for RDMA traffic. Minimum MTU is 576 for IPv4 and 1280 for IPv6\n",
> + mtu);
> + else if (mtu < IRDMA_MIN_MTU_IPV6)
> + pr_warn("Current MTU setting of %d is too low for IPv6 RDMA traffic, the minimum is 1280\n",
> + mtu);
> +}
Don't use pr_* stuff in drivers that have a struct device.
> +/**
> + * 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?
> + ldev->ops->reg_for_notification(ldev, &events);
> + dev_info(rfdev_to_dev(dev), "IRDMA VSI Open Successful");
Lets not do this kind of logging..
> +static void irdma_close(struct iidc_peer_dev *ldev, enum iidc_close_reason reason)
> +{
> + struct irdma_device *iwdev;
> + struct irdma_pci_f *rf;
> +
> + iwdev = irdma_get_device(ldev->netdev);
> + if (!iwdev)
> + return;
> +
> + irdma_put_device(iwdev);
> + rf = iwdev->rf;
> + if (reason == IIDC_REASON_GLOBR_REQ || reason == IIDC_REASON_CORER_REQ ||
> + reason == IIDC_REASON_PFR_REQ || rf->reset) {
> + iwdev->reset = true;
> + rf->reset = true;
> + }
> +
> + if (iwdev->init_state >= CEQ0_CREATED)
> + irdma_deinit_rt_device(iwdev);
> +
> + kfree(iwdev);
Mixing put and kfree? So confusing. Why are there so many structs and
so much indirection? Very hard to understand if this is right or not.
> 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
> +static struct workqueue_struct *irdma_wq;
Another wq already?
> +struct irdma_pci_f {
> + bool ooo;
> + bool reset;
> + bool rsrc_created;
> + bool stop_cqp_thread;
> + bool msix_shared;
Linus has spoken poorly about lots of bools in a struct. Can this be a
bitfield?
> +/***********************************************************/
> +/**
> + * to_iwdev - get device
> + * @ibdev: ib device
> + **/
Maybe some of these comment blocks are not so valuable :\
> + spin_lock_irqsave(&rf->rsrc_lock, flags);
> +
> + bit_is_set = test_bit(rsrc_num, rsrc_array);
Again, are these atomics? Looks like no, why test_bit?
Jason
next prev parent reply other threads:[~2019-12-10 19:04 UTC|newest]
Thread overview: 62+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-12-09 22:49 [net-next v3 00/20][pull request] Intel Wired LAN Driver Updates 2019-12-09 Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 01/20] virtual-bus: Implementation of Virtual Bus Jeff Kirsher
2019-12-10 6:49 ` Leon Romanovsky
2019-12-10 17:51 ` Jason Gunthorpe
2019-12-10 15:20 ` Greg KH
2019-12-10 18:22 ` Jason Gunthorpe
2019-12-09 22:49 ` [PATCH v3 02/20] ice: Initialize and register a virtual bus to provide RDMA Jeff Kirsher
2019-12-10 15:32 ` Greg KH
2019-12-23 19:06 ` Allan, Bruce W
2019-12-09 22:49 ` [PATCH v3 03/20] ice: Implement peer communications Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 04/20] i40e: Register a virtbus device to provide RDMA Jeff Kirsher
2019-12-10 15:33 ` Greg KH
2019-12-10 15:39 ` Greg KH
2019-12-13 23:08 ` Saleem, Shiraz
2019-12-14 8:37 ` Greg KH
2019-12-18 18:57 ` Saleem, Shiraz
2019-12-18 19:20 ` Jason Gunthorpe
2020-01-02 16:01 ` Saleem, Shiraz
2019-12-19 8:46 ` 'Greg KH'
2019-12-16 3:48 ` Parav Pandit
2019-12-16 7:15 ` Greg KH
2019-12-16 8:36 ` Parav Pandit
2019-12-16 8:58 ` Greg KH
2019-12-16 9:17 ` Parav Pandit
2019-12-09 22:49 ` [PATCH v3 05/20] RDMA/irdma: Add driver framework definitions Jeff Kirsher
2019-12-10 19:04 ` Jason Gunthorpe [this message]
2019-12-11 6:07 ` Leon Romanovsky
2019-12-12 1:40 ` Saleem, Shiraz
2019-12-12 8:39 ` Leon Romanovsky
2019-12-12 9:12 ` gregkh
2019-12-17 21:00 ` Jason Gunthorpe
2019-12-21 0:00 ` Keller, Jacob E
2019-12-09 22:49 ` [PATCH v3 06/20] RDMA/irdma: Implement device initialization definitions Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 07/20] RDMA/irdma: Implement HW Admin Queue OPs Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 08/20] RDMA/irdma: Add HMC backing store setup functions Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 09/20] RDMA/irdma: Add privileged UDA queue implementation Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 10/20] RDMA/irdma: Add QoS definitions Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 11/20] RDMA/irdma: Add connection manager Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 12/20] RDMA/irdma: Add PBLE resource manager Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 13/20] RDMA/irdma: Implement device supported verb APIs Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 14/20] RDMA/irdma: Add RoCEv2 UD OP support Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 15/20] RDMA/irdma: Add user/kernel shared libraries Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 16/20] RDMA/irdma: Add miscellaneous utility definitions Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 17/20] RDMA/irdma: Add dynamic tracing for CM Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 18/20] RDMA/irdma: Add ABI definitions Jeff Kirsher
2019-12-09 22:49 ` [PATCH v3 19/20] RDMA: Add irdma Kconfig/Makefile and remove i40iw Jeff Kirsher
2019-12-11 20:02 ` Jason Gunthorpe
2019-12-13 23:06 ` Saleem, Shiraz
2019-12-17 21:04 ` Jason Gunthorpe
2020-01-02 16:00 ` Saleem, Shiraz
2020-01-02 17:04 ` Jason Gunthorpe
2020-01-02 17:50 ` Saleem, Shiraz
2020-01-02 18:06 ` Jason Gunthorpe
2019-12-09 22:49 ` [PATCH v3 20/20] RDMA/irdma: Update MAINTAINERS file Jeff Kirsher
2019-12-10 7:33 ` [net-next v3 00/20][pull request] Intel Wired LAN Driver Updates 2019-12-09 Greg KH
2019-12-10 17:22 ` Jason Gunthorpe
2019-12-10 18:06 ` Jeff Kirsher
2019-12-10 18:25 ` Jason Gunthorpe
2019-12-10 18:41 ` Jeff Kirsher
2019-12-10 19:11 ` Jason Gunthorpe
2019-12-10 19:23 ` Jeff Kirsher
2019-12-10 19:44 ` Jason Gunthorpe
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20191210190438.GF46@ziepe.ca \
--to=jgg@ziepe$(echo .)ca \
--cc=davem@davemloft$(echo .)net \
--cc=gregkh@linuxfoundation$(echo .)org \
--cc=jeffrey.t.kirsher@intel$(echo .)com \
--cc=linux-rdma@vger$(echo .)kernel.org \
--cc=mustafa.ismail@intel$(echo .)com \
--cc=netdev@vger$(echo .)kernel.org \
--cc=nhorman@redhat$(echo .)com \
--cc=parav@mellanox$(echo .)com \
--cc=sassmann@redhat$(echo .)com \
--cc=shiraz.saleem@intel$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox