| Message ID | 20220809151636.788-1-mrgolin@amazon.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [for-next] RDMA/efa: Support CQ receive entries with source GID | expand |
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@amazon.com> > Reviewed-by: Yossi Leybovich <sleybo@amazon.com> > Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> > Signed-off-by: Michael Margolin <mrgolin@amazon.com> > --- > 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.
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@amazon.com> >> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> >> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> >> Signed-off-by: Michael Margolin <mrgolin@amazon.com> >> --- >> 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
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@amazon.com> >>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> >>> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> >>> Signed-off-by: Michael Margolin <mrgolin@amazon.com> >>> --- >>> 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.
On Tue, Aug 09, 2022 at 06:16:36PM +0300, 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@amazon.com> > Reviewed-by: Yossi Leybovich <sleybo@amazon.com> > Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> > Signed-off-by: Michael Margolin <mrgolin@amazon.com> > --- > 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_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h > index c33010bbf9e8..c6234336543d 100644 > --- a/drivers/infiniband/hw/efa/efa_com_cmd.h > +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h > @@ -76,6 +76,7 @@ struct efa_com_create_cq_params { > u16 eqn; > u8 entry_size_in_bytes; > bool interrupt_mode_enabled; > + bool set_src_addr; Please use "u8 xxx : 1" instead of bool in structs. Thanks
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@amazon.com> >>>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> >>>> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> >>>> Signed-off-by: Michael Margolin <mrgolin@amazon.com> >>>> --- >>>> 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.
On 8/16/2022 4:56 PM, Leon Romanovsky 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 Tue, Aug 09, 2022 at 06:16:36PM +0300, 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@amazon.com> >> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> >> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> >> Signed-off-by: Michael Margolin <mrgolin@amazon.com> >> --- >> 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_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h >> index c33010bbf9e8..c6234336543d 100644 >> --- a/drivers/infiniband/hw/efa/efa_com_cmd.h >> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h >> @@ -76,6 +76,7 @@ struct efa_com_create_cq_params { >> u16 eqn; >> u8 entry_size_in_bytes; >> bool interrupt_mode_enabled; >> + bool set_src_addr; > Please use "u8 xxx : 1" instead of bool in structs. > > Thanks Thanks Leon for your reply. Is this a convention in the subsystem? This is an internal struct used only to bind several variables for a function call and I think using bool makes it more readable. Of course if it's essential I will change this. Michael
On Wed, Aug 17, 2022 at 03:18:01PM +0300, Margolin, Michael wrote: > > On 8/16/2022 4:56 PM, Leon Romanovsky 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 Tue, Aug 09, 2022 at 06:16:36PM +0300, 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@amazon.com> > >> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> > >> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> > >> Signed-off-by: Michael Margolin <mrgolin@amazon.com> > >> --- > >> 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_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h > >> index c33010bbf9e8..c6234336543d 100644 > >> --- a/drivers/infiniband/hw/efa/efa_com_cmd.h > >> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h > >> @@ -76,6 +76,7 @@ struct efa_com_create_cq_params { > >> u16 eqn; > >> u8 entry_size_in_bytes; > >> bool interrupt_mode_enabled; > >> + bool set_src_addr; > > Please use "u8 xxx : 1" instead of bool in structs. > > > > Thanks > > Thanks Leon for your reply. > > Is this a convention in the subsystem? This is an internal struct used > only to bind several variables for a function call and I think using > bool makes it more readable. > > Of course if it's essential I will change this. You should use bool xx:1 in cases like this and join all the bools in your struct into a bitfield - unless you can justify them being split eg due to needing a READ_ONCE/etc or something This is expected across the kernel. Jason
On 8/17/2022 3:29 PM, Jason Gunthorpe 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 Wed, Aug 17, 2022 at 03:18:01PM +0300, Margolin, Michael wrote: >> On 8/16/2022 4:56 PM, Leon Romanovsky 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 Tue, Aug 09, 2022 at 06:16:36PM +0300, 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@amazon.com> >>>> Reviewed-by: Yossi Leybovich <sleybo@amazon.com> >>>> Signed-off-by: Daniel Kranzdorf <dkkranzd@amazon.com> >>>> Signed-off-by: Michael Margolin <mrgolin@amazon.com> >>>> --- >>>> 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_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h >>>> index c33010bbf9e8..c6234336543d 100644 >>>> --- a/drivers/infiniband/hw/efa/efa_com_cmd.h >>>> +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h >>>> @@ -76,6 +76,7 @@ struct efa_com_create_cq_params { >>>> u16 eqn; >>>> u8 entry_size_in_bytes; >>>> bool interrupt_mode_enabled; >>>> + bool set_src_addr; >>> Please use "u8 xxx : 1" instead of bool in structs. >>> >>> Thanks >> Thanks Leon for your reply. >> >> Is this a convention in the subsystem? This is an internal struct used >> only to bind several variables for a function call and I think using >> bool makes it more readable. >> >> Of course if it's essential I will change this. > You should use bool xx:1 in cases like this and join all the bools in > your struct into a bitfield - unless you can justify them being split > eg due to needing a READ_ONCE/etc or something > > This is expected across the kernel. > > Jason Thank you for your clarification, seems reasonable to me. Sending v2 patch with all the requested changes. Michael
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); + } efa_com_set_dma_addr(params->dma_addr, &create_cmd.cq_ba.mem_addr_high, &create_cmd.cq_ba.mem_addr_low); diff --git a/drivers/infiniband/hw/efa/efa_com_cmd.h b/drivers/infiniband/hw/efa/efa_com_cmd.h index c33010bbf9e8..c6234336543d 100644 --- a/drivers/infiniband/hw/efa/efa_com_cmd.h +++ b/drivers/infiniband/hw/efa/efa_com_cmd.h @@ -76,6 +76,7 @@ struct efa_com_create_cq_params { u16 eqn; u8 entry_size_in_bytes; bool interrupt_mode_enabled; + bool set_src_addr; }; struct efa_com_create_cq_result { diff --git a/drivers/infiniband/hw/efa/efa_verbs.c b/drivers/infiniband/hw/efa/efa_verbs.c index ecfe70eb5efb..c06669ca9e1f 100644 --- a/drivers/infiniband/hw/efa/efa_verbs.c +++ b/drivers/infiniband/hw/efa/efa_verbs.c @@ -1,6 +1,6 @@ // SPDX-License-Identifier: GPL-2.0 OR Linux-OpenIB /* - * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved. + * Copyright 2018-2022 Amazon.com, Inc. or its affiliates. All rights reserved. */ #include <linux/dma-buf.h> @@ -242,6 +242,7 @@ int efa_query_device(struct ib_device *ibdev, resp.max_rq_wr = dev_attr->max_rq_depth; resp.max_rdma_size = dev_attr->max_rdma_size; + resp.device_caps |= EFA_QUERY_DEVICE_CAPS_CQ_WITH_SGID; if (EFA_DEV_CAP(dev, RDMA_READ)) resp.device_caps |= EFA_QUERY_DEVICE_CAPS_RDMA_READ; @@ -1138,6 +1139,7 @@ int efa_create_cq(struct ib_cq *ibcq, const struct ib_cq_init_attr *attr, params.dma_addr = cq->dma_addr; params.entry_size_in_bytes = cmd.cq_entry_size; params.num_sub_cqs = cmd.num_sub_cqs; + params.set_src_addr = !!(cmd.flags & EFA_CREATE_CQ_WITH_SGID); if (cmd.flags & EFA_CREATE_CQ_WITH_COMPLETION_CHANNEL) { cq->eq = efa_vec2eq(dev, attr->comp_vector); params.eqn = cq->eq->eeq.eqn; diff --git a/include/uapi/rdma/efa-abi.h b/include/uapi/rdma/efa-abi.h index 08035ccf1fff..163ac79556d6 100644 --- a/include/uapi/rdma/efa-abi.h +++ b/include/uapi/rdma/efa-abi.h @@ -1,6 +1,6 @@ /* SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-2-Clause) */ /* - * Copyright 2018-2021 Amazon.com, Inc. or its affiliates. All rights reserved. + * Copyright 2018-2022 Amazon.com, Inc. or its affiliates. All rights reserved. */ #ifndef EFA_ABI_USER_H @@ -54,6 +54,7 @@ struct efa_ibv_alloc_pd_resp { enum { EFA_CREATE_CQ_WITH_COMPLETION_CHANNEL = 1 << 0, + EFA_CREATE_CQ_WITH_SGID = 1 << 1, }; struct efa_ibv_create_cq { @@ -118,6 +119,7 @@ enum { EFA_QUERY_DEVICE_CAPS_RDMA_READ = 1 << 0, EFA_QUERY_DEVICE_CAPS_RNR_RETRY = 1 << 1, EFA_QUERY_DEVICE_CAPS_CQ_NOTIFICATIONS = 1 << 2, + EFA_QUERY_DEVICE_CAPS_CQ_WITH_SGID = 1 << 3, }; struct efa_ibv_ex_query_device_resp {