On 8/15/2022 9:17 AM, Gal Pressman wrote:
> CAUTION: This email originated from outside of the organization. Do not click links or open attachments unless you can confirm the sender and know the content is safe.
>
>
>
> On 14/08/2022 18:04, Margolin, Michael wrote:
>> On 8/14/2022 2:38 PM, Gal Pressman wrote:
>>> On 09/08/2022 18:16, Michael Margolin wrote:
>>>> Add a parameter for create CQ admin command to set source address on
>>>> receive completion descriptors. Report capability for this feature
>>>> through query device verb.
>>>>
>>>> Reviewed-by: Firas Jahjah <firasj@xxxxxxxxxx>
>>>> Reviewed-by: Yossi Leybovich <sleybo@xxxxxxxxxx>
>>>> Signed-off-by: Daniel Kranzdorf <dkkranzd@xxxxxxxxxx>
>>>> Signed-off-by: Michael Margolin <mrgolin@xxxxxxxxxx>
>>>> ---
>>>> drivers/infiniband/hw/efa/efa_admin_cmds_defs.h | 6 +++++-
>>>> drivers/infiniband/hw/efa/efa_com_cmd.c | 5 ++++-
>>>> drivers/infiniband/hw/efa/efa_com_cmd.h | 1 +
>>>> drivers/infiniband/hw/efa/efa_verbs.c | 4 +++-
>>>> include/uapi/rdma/efa-abi.h | 4 +++-
>>>> 5 files changed, 16 insertions(+), 4 deletions(-)
>>>>
>>>> diff --git a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>>> index 0b0b93b529f3..d4b9226088bd 100644
>>>> --- a/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>>> +++ b/drivers/infiniband/hw/efa/efa_admin_cmds_defs.h
>>>> @@ -444,7 +444,10 @@ struct efa_admin_create_cq_cmd {
>>>> /*
>>>> * 4:0 : cq_entry_size_words - size of CQ entry in
>>>> * 32-bit words, valid values: 4, 8.
>>>> - * 7:5 : reserved7 - MBZ
>>>> + * 5 : set_src_addr - If set, source address will be
>>>> + * filled on RX completions from unknown senders.
>>>> + * Requires 8 words CQ entry size.
>>>> + * 7:6 : reserved7 - MBZ
>>>> */
>>>> u8 cq_caps_2;
>>>>
>>>> @@ -980,6 +983,7 @@ struct efa_admin_host_info {
>>>> #define EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED_MASK BIT(5)
>>>> #define EFA_ADMIN_CREATE_CQ_CMD_VIRT_MASK BIT(6)
>>>> #define EFA_ADMIN_CREATE_CQ_CMD_CQ_ENTRY_SIZE_WORDS_MASK GENMASK(4, 0)
>>>> +#define EFA_ADMIN_CREATE_CQ_CMD_SET_SRC_ADDR_MASK BIT(5)
>>>>
>>>> /* create_cq_resp */
>>>> #define EFA_ADMIN_CREATE_CQ_RESP_DB_VALID_MASK BIT(0)
>>>> diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.c b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>>> index fb405da4e1db..8f8885e002ba 100644
>>>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.c
>>>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.c
>>>> @@ -168,7 +168,10 @@ int efa_com_create_cq(struct efa_com_dev *edev,
>>>> EFA_ADMIN_CREATE_CQ_CMD_INTERRUPT_MODE_ENABLED, 1);
>>>> create_cmd.eqn = params->eqn;
>>>> }
>>>> -
>>>> + if (params->set_src_addr) {
>>>> + EFA_SET(&create_cmd.cq_caps_2,
>>>> + EFA_ADMIN_CREATE_CQ_CMD_SET_SRC_ADDR, 1);
>>>> + }
>>> Don't you need to validate the CQE size requested by the user somewhere?
>>> I assume you must use 32 bytes completions for this.
>> This is a good point. Requested CQE size is validated against command
>> feature bits in the device FW. In a case of user requesting for a wrong
>> or unsupported configuration efa_com_cmd_exec() will return an
>> appropriate error code. This is to avoid driver dependency on CQE
>> structure or size.
>>
>>
>> Michael
>>
> The driver usually terminates bad inputs before they get to the device
> to prevent unnecessary noise.
> As of today, command failures are unexpected so we have a ibdev_err to
> indicate that something went very wrong, I don't think you want to allow
> a non-root user to flood your device (and dmesg) with errors.
I agree that in this case it's better to just return an error code
rather than printing to dmesg.
To avoid greater changes in error handling for control over error
printing, I'll go with your suggestion and check for command correctness
in driver code. Anyway it's better to early check all inputs.
Thanks.
