RE: [PATCH net-next] net: mana: Add a driver for Microsoft Azure Network Adapter (MANA)

[Dexuan Cui <decui@microsoft.com>]

> From: Andrew Lunn <andrew@lunn.ch>
> Sent: Tuesday, April 6, 2021 6:08 PM
> To: Dexuan Cui <decui@microsoft.com>
> 
> > +static int gdma_query_max_resources(struct pci_dev *pdev)
> > +{
> > +	struct gdma_context *gc = pci_get_drvdata(pdev);
> > +	struct gdma_general_req req = { 0 };
> > +	struct gdma_query_max_resources_resp resp = { 0 };
> > +	int err;
> 
> Network drivers need to use reverse christmas tree. I spotted a number
> of functions getting this wrong. Please review the whole driver.

Hi Andrew, thanks for the quick comments!

I think In general the patch follows the reverse christmas tree style.

For the function you pointed out, I didn't follow the reverse
christmas tree style because I wanted to keep the variable defines
more natural, i.e. first define 'req' and then 'resp'.

I can swap the 2 lines here, i.e. define 'resp' first, but this looks a little
strange to me, because in some other functions the 'req' should be
defined first, e.g. 

int gdma_test_eq(struct gdma_context *gc, struct gdma_queue *eq)
{
        struct gdma_generate_test_event_req req = { 0 };
        struct gdma_general_resp resp = { 0 };


And, some variable defines can not follow the reverse christmas tree
style due to dependency, e.g.
static void hwc_init_event_handler(void *ctx, struct gdma_queue *q_self,
                                   struct gdma_event *event) 
{
        struct hw_channel_context *hwc = ctx;
        struct gdma_dev *gd = hwc->gdma_dev;
        struct gdma_context *gc = gdma_dev_to_context(gd);

I failed to find the reverse christmas tree rule in the Documentation/ 
folder. I knew the rule and I thought it was documented there,
but it looks like it's not. So my understanding is that in general we
should follow the style, but there can be exceptions due to
dependencies or logical grouping.

> > +	gdma_init_req_hdr(&req.hdr, GDMA_QUERY_MAX_RESOURCES,
> > +			  sizeof(req), sizeof(resp));
> > +
> > +	err = gdma_send_request(gc, sizeof(req), &req, sizeof(resp), &resp);
> > +	if (err || resp.hdr.status) {
> > +		pr_err("%s, line %d: err=%d, err=0x%x\n", __func__, __LINE__,
> > +		       err, resp.hdr.status);
> 
> I expect checkpatch complained about this. Don't use pr_err(), use
> dev_err(pdev->dev, ...  of netdev_err(ndev, ... You should always have
> access to dev or ndev, so please change all pr_ calls.

I did run scripts/checkpatch.pl and it reported no error/warning. :-)

But I think you're correct. I'll change the pr_* to dev_* or netdev_*.
 
> > +static unsigned int num_queues = ANA_DEFAULT_NUM_QUEUE;
> > +module_param(num_queues, uint, 0444);
> 
> No module parameters please.
> 
>    Andrew

This parameter was mostly for debugging purpose. I can remove it.

BTW, I do remember some maintainers ask people to not add module
parameters unless that's absolutely necessary, but I don't remember if
the rule applies to only the net subsystem or to the whole drivers/
folder. It looks like the rule was also not documented in the
Documentation/ folder? Please let me know if such a rule is explicitly
defined somewhere.

Thanks,
-- Dexuan