Re: [RFC PATCH 5/8] qaic: Implement data path

[Jeffrey Hugo <jhugo@codeaurora.org>]

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@codeaurora.org> 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.