Thanks for the review.
On 5/14/2020 3:36 PM, Arnd Bergmann wrote:
On Thu, May 14, 2020 at 4:09 PM Jeffrey Hugo <jhugo@xxxxxxxxxxxxxx> wrote:
+struct dbc_req { /* everything must be little endian encoded */
Instead of the comment, I suppose you want to use __le16 and __le32
types and let sparse check that you got it right.
Ah yes, I was curious if those should be applied here. Their use seems
inconsistent. I will do that.
+ u16 req_id;
+ u8 seq_id;
+ u8 cmd;
+ u32 resv;
+ u64 src_addr;
+ u64 dest_addr;
+ u32 len;
+ u32 resv2;
+ u64 db_addr; /* doorbell address */
+ u8 db_len; /* not a raw value, special encoding */
+ u8 resv3;
+ u16 resv4;
+ u32 db_data;
+ u32 sem_cmd0;
+ u32 sem_cmd1;
+ u32 sem_cmd2;
+ u32 sem_cmd3;
+} __packed;
All members are naturally aligned, so better drop the __packed
annotation get better code, unless the structure itself is
at an unaligned offset in memory.
I'm going to have to disagree. While most "sane" compilers would not
add extra padding, I've debugged enough issues in the past when
sending/receiving data with foreign environments to never trust anything
that isn't "packed".
Unless I missed something in the C spec that requires naturally aligned
structures to have an identical layout in memory, I'll take safety and
functional correctness over performance.
+struct dbc_rsp { /* everything must be little endian encoded */
+ u16 req_id;
+ u16 status;
+} __packed;
Same here.
+ init_completion(&mem->xfer_done);
+ list_add_tail(&mem->list, &dbc->xfer_list);
+ tail = (tail + mem->nents) % dbc->nelem;
+ __raw_writel(cpu_to_le32(tail), dbc->dbc_base + REQTP_OFF);
What is this __raw accessor for? This generally results in non-portable
code that should be replaced with writel() or something specific to
the bus on the architecture you deal with.
The barrier(s) that comes with writel are unnecessary in this case.
Since this is part of our critical path, we are sensitive to its
performance.
What are the portability issues around the __raw variant?
+ spin_lock_irqsave(&qdev->dbc[exec->dbc_id].xfer_lock, flags);
+ req_id = qdev->dbc[exec->dbc_id].next_req_id++;
+ queued = mem->queued;
+ mem->queued = true;
+ spin_unlock_irqrestore(&qdev->dbc[exec->dbc_id].xfer_lock, flags);
No need for 'irqsave' locks when you know that interrupts are enabled.
Fair enough.
+ head = le32_to_cpu(__raw_readl(dbc->dbc_base + RSPHP_OFF));
+ tail = le32_to_cpu(__raw_readl(dbc->dbc_base + RSPTP_OFF));
More __raw accessors to replace.
Same answer as before.
+ case QAIC_IOCTL_MEM_NR:
+ if (_IOC_DIR(cmd) != (_IOC_READ | _IOC_WRITE) ||
+ _IOC_SIZE(cmd) != sizeof(struct qaic_mem_req)) {
+ ret = -EINVAL;
+ break;
This looks like a very verbose way to check 'cmd' against a known
constant. Why not use 'switch (cmd)' like all other drivers?
Huh. That actually does sound more elegant. Will do.
--
Jeffrey Hugo
Qualcomm Technologies, Inc. is a member of the
Code Aurora Forum, a Linux Foundation Collaborative Project.
