From: Christoph Hellwig <hch@lst.de>
To: hare@kernel.org
Cc: Christoph Hellwig <hch@lst.de>, Sagi Grimberg <sagi@grimberg.me>,
Keith Busch <kbusch@kernel.org>,
linux-nvme@lists.infradead.org, Hannes Reinecke <hare@suse.de>
Subject: Re: [PATCH v2] nvme-fc: FPIN link integrity handling
Date: Tue, 2 Apr 2024 14:55:34 +0200 [thread overview]
Message-ID: <20240402125534.GA31497@lst.de> (raw)
In-Reply-To: <20240402093031.146342-1-hare@kernel.org>
> +static struct nvme_fc_rport *nvme_fc_rport_from_wwpn(struct nvme_fc_lport *lport,
> + u64 rport_wwpn)
Pleae fix all this whacky formatting. Avoid the overly long lines
(in this case by just having the return value on it's own line)
and use two tab continuations instead of wasting most of the continuing
line.
> +/*
> + * nvme_fc_fpin_li_lport_update - routine to update Link Integrity
> + * event statistics.
> + * @lport: local port the FPIN was received on
> + * @tlv: pointer to link integrity descriptor
> + *
> + */
What's the point of this half-kernel doc comment? It doesn't really
add much valye for a local function.
> +static void
> +nvme_fc_fpin_li_lport_update(struct nvme_fc_lport *lport, struct fc_tlv_desc *tlv)
> +{
> + unsigned int i, pname_count;
> + struct nvme_fc_rport *attached_rport;
> + struct fc_fn_li_desc *li_desc = (struct fc_fn_li_desc *)tlv;
The casting here is pretty gross. Sounds like the FC code should use
a union or better fix fc_fn_li_desc to not duplicate the tag/len
fields?
> + u64 wwpn;
> +
> + wwpn = be64_to_cpu(li_desc->attached_wwpn);
> + attached_rport = nvme_fc_rport_from_wwpn(lport, wwpn);
> + pname_count = be32_to_cpu(li_desc->pname_count);
... and if these were initialized at declartion time thing would
get a lot easier to read.
> +EXPORT_SYMBOL(nvme_fc_fpin_rcv);
All nvme exports are EXPORT_SYMBOL_GPL
Also please split the patch up into nvme core, nvme-fc and
driver changes.
prev parent reply other threads:[~2024-04-02 12:55 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-02 9:30 [PATCH v2] nvme-fc: FPIN link integrity handling hare
2024-04-02 12:55 ` Christoph Hellwig [this message]
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=20240402125534.GA31497@lst.de \
--to=hch@lst.de \
--cc=hare@kernel.org \
--cc=hare@suse.de \
--cc=kbusch@kernel.org \
--cc=linux-nvme@lists.infradead.org \
--cc=sagi@grimberg.me \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
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;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).