Re: [PATCH for-next v4 06/12] RDMA/erdma: Add event queue implementation

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 





On 3/22/22 6:23 AM, Jason Gunthorpe wrote:
On Sat, Mar 19, 2022 at 05:43:16PM +0800, Cheng Xu wrote:


On 3/19/22 2:18 AM, Jason Gunthorpe wrote:
On Fri, Mar 18, 2022 at 07:43:21PM +0800, Wenpeng Liang wrote:

+static int erdma_set_ceq_irq(struct erdma_dev *dev, u16 ceqn)
+{
+	struct erdma_eq_cb *eqc = &dev->ceqs[ceqn];
+	cpumask_t affinity_hint_mask;
+	u32 cpu;
+	int err;
+
+	snprintf(eqc->irq_name, ERDMA_IRQNAME_SIZE, "erdma-ceq%u@pci:%s",
+		ceqn, pci_name(dev->pdev));

Parameters in parentheses are not vertically aligned, a space is missing before "ceqn".

Generally I will recommend such a large amount of code be run through
clang-format and the good things it changes be merged. Most of what it
suggests is good kernel style

Jason

Thanks, Jason. We already use clang-format since you recommended it last
time.

We review the changes made by clang-format case by case, and merge most
of the changes. With a few cases, we handle them manually.

For this case, clang-format put the ceqn at the first line, making it too
long (80 chars), and then the two lines of snprintf look like not
balance. So, I put the ceqn to the second line and broke the alignment
by mistake.

That sounds strange, clang-format never makes lines too long unless
strings are invovled...


Maybe I didn't explain this clearly.

The clang-format's result is well and matches the rule, the result looks
like this:

line 1:	snprintf(..., ..., ..., ceqn,     // 80 chars
line 2:		 pci_name(dev->pdev));    // 39 chars

The line 1 (80 chars) looks much longer than line 2 (39 chars). So I
put the 'ceqn' at line 2, then the length of the two lines are more
close, and may looks better:

line 1:	snprintf(..., ..., ...,                // 74 chars
line 2:		 ceqn, pci_name(dev->pdev));   // 45 chars

Thanks,
Cheng Xu

Jason



[Index of Archives]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Photo]     [Yosemite News]     [Yosemite Photos]     [Linux Kernel]     [Linux SCSI]     [XFree86]

  Powered by Linux