Message-ID: <20191210190438.GF46@ziepe.ca>
Date:   Tue, 10 Dec 2019 15:04:38 -0400
From:   Jason Gunthorpe <jgg@...pe.ca>
To:     Jeff Kirsher <jeffrey.t.kirsher@...el.com>
Cc:     davem@...emloft.net, gregkh@...uxfoundation.org,
        Mustafa Ismail <mustafa.ismail@...el.com>,
        netdev@...r.kernel.org, linux-rdma@...r.kernel.org,
        nhorman@...hat.com, sassmann@...hat.com, parav@...lanox.com,
        Shiraz Saleem <shiraz.saleem@...el.com>
Subject: Re: [PATCH v3 05/20] RDMA/irdma: Add driver framework definitions

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

Powered by blists - more mailing lists