On Tue, Jun 18, 2024 at 07:58:55AM +0000, Omer Shpigelman wrote: > On 6/18/24 10:08, Leon Romanovsky wrote: > > On Tue, Jun 18, 2024 at 05:50:15AM +0000, Omer Shpigelman wrote: > >> On 6/17/24 16:18, Leon Romanovsky wrote: > >>> [Some people who received this message don't often get email from leon@xxxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ] > >>> > >>> On Thu, Jun 13, 2024 at 11:21:57AM +0300, Omer Shpigelman wrote: > >>>> Add a common QP state machine which handles the moving for a QP from one > >>>> state to another including performing necessary checks, draining > >>>> in-flight transactions, invalidating caches and error reporting. > >>>> > >>>> Signed-off-by: Omer Shpigelman <oshpigelman@xxxxxxxxx> > >>>> Co-developed-by: Abhilash K V <kvabhilash@xxxxxxxxx> > >>>> Signed-off-by: Abhilash K V <kvabhilash@xxxxxxxxx> > >>>> Co-developed-by: Andrey Agranovich <aagranovich@xxxxxxxxx> > >>>> Signed-off-by: Andrey Agranovich <aagranovich@xxxxxxxxx> > >>>> Co-developed-by: Bharat Jauhari <bjauhari@xxxxxxxxx> > >>>> Signed-off-by: Bharat Jauhari <bjauhari@xxxxxxxxx> > >>>> Co-developed-by: David Meriin <dmeriin@xxxxxxxxx> > >>>> Signed-off-by: David Meriin <dmeriin@xxxxxxxxx> > >>>> Co-developed-by: Sagiv Ozeri <sozeri@xxxxxxxxx> > >>>> Signed-off-by: Sagiv Ozeri <sozeri@xxxxxxxxx> > >>>> Co-developed-by: Zvika Yehudai <zyehudai@xxxxxxxxx> > >>>> Signed-off-by: Zvika Yehudai <zyehudai@xxxxxxxxx> > >>>> --- > >>>> .../ethernet/intel/hbl_cn/common/hbl_cn_qp.c | 480 +++++++++++++++++- > >>>> 1 file changed, 479 insertions(+), 1 deletion(-) > >>>> > >>>> diff --git a/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c b/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c > >>>> index 9ddc23bf8194..26ebdf448193 100644 > >>>> --- a/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c > >>>> +++ b/drivers/net/ethernet/intel/hbl_cn/common/hbl_cn_qp.c > >>>> @@ -6,8 +6,486 @@ > >>> > >>> <...> > >>> > >>>> +/* The following table represents the (valid) operations that can be performed on > >>>> + * a QP in order to move it from one state to another > >>>> + * For example: a QP in RTR state can be moved to RTS state using the CN_QP_OP_RTR_2RTS > >>>> + * operation. > >>>> + */ > >>>> +static const enum hbl_cn_qp_state_op qp_valid_state_op[CN_QP_NUM_STATE][CN_QP_NUM_STATE] = { > >>>> + [CN_QP_STATE_RESET] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_INIT] = CN_QP_OP_RST_2INIT, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, > >>>> + }, > >>>> + [CN_QP_STATE_INIT] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_INIT] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_RTR] = CN_QP_OP_INIT_2RTR, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, > >>>> + }, > >>>> + [CN_QP_STATE_RTR] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_RTR] = CN_QP_OP_RTR_2RTR, > >>>> + [CN_QP_STATE_RTS] = CN_QP_OP_RTR_2RTS, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_RTR_2QPD, > >>>> + }, > >>>> + [CN_QP_STATE_RTS] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_RTS] = CN_QP_OP_RTS_2RTS, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_RTS_2SQD, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_RTS_2QPD, > >>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_RTS_2SQERR, > >>>> + }, > >>>> + [CN_QP_STATE_SQD] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_SQD_2SQD, > >>>> + [CN_QP_STATE_RTS] = CN_QP_OP_SQD_2RTS, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_SQD_2QPD, > >>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_SQD_2SQ_ERR, > >>>> + }, > >>>> + [CN_QP_STATE_QPD] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_QPD] = CN_QP_OP_NOP, > >>>> + [CN_QP_STATE_RTR] = CN_QP_OP_QPD_2RTR, > >>>> + }, > >>>> + [CN_QP_STATE_SQERR] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + [CN_QP_STATE_SQD] = CN_QP_OP_SQ_ERR_2SQD, > >>>> + [CN_QP_STATE_SQERR] = CN_QP_OP_NOP, > >>>> + }, > >>>> + [CN_QP_STATE_ERR] = { > >>>> + [CN_QP_STATE_RESET] = CN_QP_OP_2RESET, > >>>> + [CN_QP_STATE_ERR] = CN_QP_OP_2ERR, > >>>> + } > >>>> +}; > >>> > >>> I don't understand why IBTA QP state machine is declared in ETH driver > >>> and not in IB driver. > >>> > >> > >> Implementing the actual transitions between the states requires full > >> knowledge of the HW e.g. when to flush, cache invalidation, timeouts. > >> Our IB driver is agnostic to the ASIC type by design. Note that more ASIC > >> generations are planned to be added and the IB driver should not be aware > >> of these additional HWs. > >> Hence we implemeted the QP state machine in the CN driver which is aware > >> of the actual HW. > > > > Somehow ALL other IB drivers are able to implement this logic in the IB, > > while supporting multiple ASICs. I don't see a reason why you can't do > > the same. > > > > If we are referring to this actual table, then I can move it to the IB > driver and the CN driver will fetch the needed opcode via a function > pointer. > Is that ok? This table spotted my attention, but right separation shouldn't be limited to only this table. The outcome of this conversation should be: "IB specific logic should be in IB driver, and CN driver should be able to handle only low-level operations". Thanks
