* [PATCH 0/3] Add MCTP-over-KCS transport binding
@ 2023-09-28 12:30 Konstantin Aladyshev
2023-09-28 12:30 ` [PATCH 1/3] ipmi: Move KCS headers to common include folder Konstantin Aladyshev
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Konstantin Aladyshev @ 2023-09-28 12:30 UTC (permalink / raw)
Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1,
venture, yuenn, benjaminfair, aladyshev22, jk, matt, davem,
edumazet, kuba, pabeni, openipmi-developer, linux-kernel,
linux-arm-kernel, linux-aspeed, openbmc, netdev
This change adds a MCTP KCS transport binding, as defined by the DMTF
specificiation DSP0254 - "MCTP KCS Transport Binding".
A MCTP protocol network device is created for each KCS channel found in
the system.
The interrupt code for the KCS state machine is based on the current
IPMI KCS driver.
Since the KCS subsystem code is now used both in IPMI and MCTP drivers
the separate patchsets move KCS subsystem includes to a common folder.
Tested:
PLDM communication between the HOST and BMC was tested with both
components implemented via open-source software:
- The HOST (UEFI firmware) part was based one the edk2 [1] and
edk2-platforms [2] code,
- The BMC part was based on the openbmc [3] distribution.
The testing process and all the necessary utilities are described in
the [4] repository.
[1]: https://github.com/tianocore/edk2
[2]: https://github.com/tianocore/edk2-platforms
[3]: https://github.com/openbmc/openbmc
[4]: https://github.com/Kostr/PLDM
Konstantin Aladyshev (3):
ipmi: Move KCS headers to common include folder
ipmi: Create header with KCS interface defines
mctp: Add MCTP-over-KCS transport binding
drivers/char/ipmi/kcs_bmc.c | 8 +-
drivers/char/ipmi/kcs_bmc_aspeed.c | 3 +-
drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 73 +-
drivers/char/ipmi/kcs_bmc_npcm7xx.c | 2 +-
drivers/char/ipmi/kcs_bmc_serio.c | 2 +-
drivers/net/mctp/Kconfig | 8 +
drivers/net/mctp/Makefile | 1 +
drivers/net/mctp/mctp-kcs.c | 624 ++++++++++++++++++
include/linux/ipmi_kcs.h | 80 +++
.../char/ipmi => include/linux}/kcs_bmc.h | 0
.../ipmi => include/linux}/kcs_bmc_client.h | 3 +-
.../ipmi => include/linux}/kcs_bmc_device.h | 3 +-
12 files changed, 723 insertions(+), 84 deletions(-)
create mode 100644 drivers/net/mctp/mctp-kcs.c
create mode 100644 include/linux/ipmi_kcs.h
rename {drivers/char/ipmi => include/linux}/kcs_bmc.h (100%)
rename {drivers/char/ipmi => include/linux}/kcs_bmc_client.h (97%)
rename {drivers/char/ipmi => include/linux}/kcs_bmc_device.h (96%)
--
2.25.1
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH 1/3] ipmi: Move KCS headers to common include folder 2023-09-28 12:30 [PATCH 0/3] Add MCTP-over-KCS transport binding Konstantin Aladyshev @ 2023-09-28 12:30 ` Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 2/3] ipmi: Create header with KCS interface defines Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding Konstantin Aladyshev 2 siblings, 0 replies; 10+ messages in thread From: Konstantin Aladyshev @ 2023-09-28 12:30 UTC (permalink / raw) Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, aladyshev22, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev The current KCS header files can be utilized by both IPMI drivers (drivers/char/ipmi) and MCTP driver (drivers/net/mctp). To be able to use them in both cases move the headers to 'include/linux' folder. Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> --- drivers/char/ipmi/kcs_bmc.c | 8 +++----- drivers/char/ipmi/kcs_bmc_aspeed.c | 3 +-- drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 2 +- drivers/char/ipmi/kcs_bmc_npcm7xx.c | 2 +- drivers/char/ipmi/kcs_bmc_serio.c | 2 +- {drivers/char/ipmi => include/linux}/kcs_bmc.h | 0 {drivers/char/ipmi => include/linux}/kcs_bmc_client.h | 3 +-- {drivers/char/ipmi => include/linux}/kcs_bmc_device.h | 3 +-- 8 files changed, 9 insertions(+), 14 deletions(-) rename {drivers/char/ipmi => include/linux}/kcs_bmc.h (100%) rename {drivers/char/ipmi => include/linux}/kcs_bmc_client.h (97%) rename {drivers/char/ipmi => include/linux}/kcs_bmc_device.h (96%) diff --git a/drivers/char/ipmi/kcs_bmc.c b/drivers/char/ipmi/kcs_bmc.c index 8b1161d5194a..d29a8505d6ed 100644 --- a/drivers/char/ipmi/kcs_bmc.c +++ b/drivers/char/ipmi/kcs_bmc.c @@ -5,15 +5,13 @@ */ #include <linux/device.h> +#include <linux/kcs_bmc.h> +#include <linux/kcs_bmc_client.h> +#include <linux/kcs_bmc_device.h> #include <linux/list.h> #include <linux/module.h> #include <linux/mutex.h> -#include "kcs_bmc.h" - -/* Implement both the device and client interfaces here */ -#include "kcs_bmc_device.h" -#include "kcs_bmc_client.h" /* Record registered devices and drivers */ static DEFINE_MUTEX(kcs_bmc_lock); diff --git a/drivers/char/ipmi/kcs_bmc_aspeed.c b/drivers/char/ipmi/kcs_bmc_aspeed.c index 72640da55380..3dc0dfb448f5 100644 --- a/drivers/char/ipmi/kcs_bmc_aspeed.c +++ b/drivers/char/ipmi/kcs_bmc_aspeed.c @@ -10,6 +10,7 @@ #include <linux/interrupt.h> #include <linux/io.h> #include <linux/irq.h> +#include <linux/kcs_bmc_device.h> #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> @@ -21,8 +22,6 @@ #include <linux/slab.h> #include <linux/timer.h> -#include "kcs_bmc_device.h" - #define DEVICE_NAME "ast-kcs-bmc" diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c index cf670e891966..bf1001130a6c 100644 --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c @@ -8,6 +8,7 @@ #include <linux/errno.h> #include <linux/io.h> #include <linux/ipmi_bmc.h> +#include <linux/kcs_bmc_client.h> #include <linux/list.h> #include <linux/miscdevice.h> #include <linux/module.h> @@ -17,7 +18,6 @@ #include <linux/sched.h> #include <linux/slab.h> -#include "kcs_bmc_client.h" /* Different phases of the KCS BMC module. * KCS_PHASE_IDLE: diff --git a/drivers/char/ipmi/kcs_bmc_npcm7xx.c b/drivers/char/ipmi/kcs_bmc_npcm7xx.c index 7961fec56476..160553248a93 100644 --- a/drivers/char/ipmi/kcs_bmc_npcm7xx.c +++ b/drivers/char/ipmi/kcs_bmc_npcm7xx.c @@ -10,6 +10,7 @@ #include <linux/errno.h> #include <linux/interrupt.h> #include <linux/io.h> +#include <linux/kcs_bmc_device.h> #include <linux/mfd/syscon.h> #include <linux/module.h> #include <linux/of.h> @@ -17,7 +18,6 @@ #include <linux/regmap.h> #include <linux/slab.h> -#include "kcs_bmc_device.h" #define DEVICE_NAME "npcm-kcs-bmc" #define KCS_CHANNEL_MAX 3 diff --git a/drivers/char/ipmi/kcs_bmc_serio.c b/drivers/char/ipmi/kcs_bmc_serio.c index 1793358be782..24df7144a189 100644 --- a/drivers/char/ipmi/kcs_bmc_serio.c +++ b/drivers/char/ipmi/kcs_bmc_serio.c @@ -5,12 +5,12 @@ #include <linux/device.h> #include <linux/errno.h> #include <linux/list.h> +#include <linux/kcs_bmc_client.h> #include <linux/module.h> #include <linux/sched/signal.h> #include <linux/serio.h> #include <linux/slab.h> -#include "kcs_bmc_client.h" struct kcs_bmc_serio { struct list_head entry; diff --git a/drivers/char/ipmi/kcs_bmc.h b/include/linux/kcs_bmc.h similarity index 100% rename from drivers/char/ipmi/kcs_bmc.h rename to include/linux/kcs_bmc.h diff --git a/drivers/char/ipmi/kcs_bmc_client.h b/include/linux/kcs_bmc_client.h similarity index 97% rename from drivers/char/ipmi/kcs_bmc_client.h rename to include/linux/kcs_bmc_client.h index 6fdcde0a7169..f6350c9366dd 100644 --- a/drivers/char/ipmi/kcs_bmc_client.h +++ b/include/linux/kcs_bmc_client.h @@ -5,8 +5,7 @@ #define __KCS_BMC_CONSUMER_H__ #include <linux/irqreturn.h> - -#include "kcs_bmc.h" +#include <linux/kcs_bmc.h> struct kcs_bmc_driver_ops { int (*add_device)(struct kcs_bmc_device *kcs_bmc); diff --git a/drivers/char/ipmi/kcs_bmc_device.h b/include/linux/kcs_bmc_device.h similarity index 96% rename from drivers/char/ipmi/kcs_bmc_device.h rename to include/linux/kcs_bmc_device.h index 17c572f25c54..65333b68c0af 100644 --- a/drivers/char/ipmi/kcs_bmc_device.h +++ b/include/linux/kcs_bmc_device.h @@ -5,8 +5,7 @@ #define __KCS_BMC_DEVICE_H__ #include <linux/irqreturn.h> - -#include "kcs_bmc.h" +#include <linux/kcs_bmc.h> struct kcs_bmc_device_ops { void (*irq_mask_update)(struct kcs_bmc_device *kcs_bmc, u8 mask, u8 enable); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] ipmi: Create header with KCS interface defines 2023-09-28 12:30 [PATCH 0/3] Add MCTP-over-KCS transport binding Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 1/3] ipmi: Move KCS headers to common include folder Konstantin Aladyshev @ 2023-09-28 12:30 ` Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding Konstantin Aladyshev 2 siblings, 0 replies; 10+ messages in thread From: Konstantin Aladyshev @ 2023-09-28 12:30 UTC (permalink / raw) Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, aladyshev22, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev Some definitions from the current kcs_bmc_cdev_ipmi driver can be also utilized by the MTCP KCS binding driver. Move such definitions to the common header file. Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> --- drivers/char/ipmi/kcs_bmc_cdev_ipmi.c | 71 +----------------------- include/linux/ipmi_kcs.h | 80 +++++++++++++++++++++++++++ 2 files changed, 81 insertions(+), 70 deletions(-) create mode 100644 include/linux/ipmi_kcs.h diff --git a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c index bf1001130a6c..f158f676114c 100644 --- a/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c +++ b/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c @@ -8,6 +8,7 @@ #include <linux/errno.h> #include <linux/io.h> #include <linux/ipmi_bmc.h> +#include <linux/ipmi_kcs.h> #include <linux/kcs_bmc_client.h> #include <linux/list.h> #include <linux/miscdevice.h> @@ -19,53 +20,6 @@ #include <linux/slab.h> -/* Different phases of the KCS BMC module. - * KCS_PHASE_IDLE: - * BMC should not be expecting nor sending any data. - * KCS_PHASE_WRITE_START: - * BMC is receiving a WRITE_START command from system software. - * KCS_PHASE_WRITE_DATA: - * BMC is receiving a data byte from system software. - * KCS_PHASE_WRITE_END_CMD: - * BMC is waiting a last data byte from system software. - * KCS_PHASE_WRITE_DONE: - * BMC has received the whole request from system software. - * KCS_PHASE_WAIT_READ: - * BMC is waiting the response from the upper IPMI service. - * KCS_PHASE_READ: - * BMC is transferring the response to system software. - * KCS_PHASE_ABORT_ERROR1: - * BMC is waiting error status request from system software. - * KCS_PHASE_ABORT_ERROR2: - * BMC is waiting for idle status afer error from system software. - * KCS_PHASE_ERROR: - * BMC has detected a protocol violation at the interface level. - */ -enum kcs_ipmi_phases { - KCS_PHASE_IDLE, - - KCS_PHASE_WRITE_START, - KCS_PHASE_WRITE_DATA, - KCS_PHASE_WRITE_END_CMD, - KCS_PHASE_WRITE_DONE, - - KCS_PHASE_WAIT_READ, - KCS_PHASE_READ, - - KCS_PHASE_ABORT_ERROR1, - KCS_PHASE_ABORT_ERROR2, - KCS_PHASE_ERROR -}; - -/* IPMI 2.0 - Table 9-4, KCS Interface Status Codes */ -enum kcs_ipmi_errors { - KCS_NO_ERROR = 0x00, - KCS_ABORTED_BY_COMMAND = 0x01, - KCS_ILLEGAL_CONTROL_CODE = 0x02, - KCS_LENGTH_ERROR = 0x06, - KCS_UNSPECIFIED_ERROR = 0xFF -}; - struct kcs_bmc_ipmi { struct list_head entry; @@ -95,29 +49,6 @@ struct kcs_bmc_ipmi { #define KCS_MSG_BUFSIZ 1000 -#define KCS_ZERO_DATA 0 - -/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */ -#define KCS_STATUS_STATE(state) (state << 6) -#define KCS_STATUS_STATE_MASK GENMASK(7, 6) -#define KCS_STATUS_CMD_DAT BIT(3) -#define KCS_STATUS_SMS_ATN BIT(2) -#define KCS_STATUS_IBF BIT(1) -#define KCS_STATUS_OBF BIT(0) - -/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */ -enum kcs_states { - IDLE_STATE = 0, - READ_STATE = 1, - WRITE_STATE = 2, - ERROR_STATE = 3, -}; - -/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */ -#define KCS_CMD_GET_STATUS_ABORT 0x60 -#define KCS_CMD_WRITE_START 0x61 -#define KCS_CMD_WRITE_END 0x62 -#define KCS_CMD_READ_BYTE 0x68 static inline void set_state(struct kcs_bmc_ipmi *priv, u8 state) { diff --git a/include/linux/ipmi_kcs.h b/include/linux/ipmi_kcs.h new file mode 100644 index 000000000000..30c4b6e4d689 --- /dev/null +++ b/include/linux/ipmi_kcs.h @@ -0,0 +1,80 @@ +/* SPDX-License-Identifier: GPL-2.0 */ +/* + * Copyright (c) 2015-2018, Intel Corporation. + */ + +#ifndef __IPMI_KCS_H__ +#define __IPMI_KCS_H__ + +/* Different phases of the KCS BMC module. + * KCS_PHASE_IDLE: + * BMC should not be expecting nor sending any data. + * KCS_PHASE_WRITE_START: + * BMC is receiving a WRITE_START command from system software. + * KCS_PHASE_WRITE_DATA: + * BMC is receiving a data byte from system software. + * KCS_PHASE_WRITE_END_CMD: + * BMC is waiting a last data byte from system software. + * KCS_PHASE_WRITE_DONE: + * BMC has received the whole request from system software. + * KCS_PHASE_WAIT_READ: + * BMC is waiting the response from the upper IPMI service. + * KCS_PHASE_READ: + * BMC is transferring the response to system software. + * KCS_PHASE_ABORT_ERROR1: + * BMC is waiting error status request from system software. + * KCS_PHASE_ABORT_ERROR2: + * BMC is waiting for idle status afer error from system software. + * KCS_PHASE_ERROR: + * BMC has detected a protocol violation at the interface level. + */ +enum kcs_ipmi_phases { + KCS_PHASE_IDLE, + + KCS_PHASE_WRITE_START, + KCS_PHASE_WRITE_DATA, + KCS_PHASE_WRITE_END_CMD, + KCS_PHASE_WRITE_DONE, + + KCS_PHASE_WAIT_READ, + KCS_PHASE_READ, + + KCS_PHASE_ABORT_ERROR1, + KCS_PHASE_ABORT_ERROR2, + KCS_PHASE_ERROR +}; + +/* IPMI 2.0 - Table 9-4, KCS Interface Status Codes */ +enum kcs_ipmi_errors { + KCS_NO_ERROR = 0x00, + KCS_ABORTED_BY_COMMAND = 0x01, + KCS_ILLEGAL_CONTROL_CODE = 0x02, + KCS_LENGTH_ERROR = 0x06, + KCS_UNSPECIFIED_ERROR = 0xFF +}; + +#define KCS_ZERO_DATA 0 + +/* IPMI 2.0 - Table 9-1, KCS Interface Status Register Bits */ +#define KCS_STATUS_STATE(state) (state << 6) +#define KCS_STATUS_STATE_MASK GENMASK(7, 6) +#define KCS_STATUS_CMD_DAT BIT(3) +#define KCS_STATUS_SMS_ATN BIT(2) +#define KCS_STATUS_IBF BIT(1) +#define KCS_STATUS_OBF BIT(0) + +/* IPMI 2.0 - Table 9-2, KCS Interface State Bits */ +enum kcs_states { + IDLE_STATE = 0, + READ_STATE = 1, + WRITE_STATE = 2, + ERROR_STATE = 3, +}; + +/* IPMI 2.0 - Table 9-3, KCS Interface Control Codes */ +#define KCS_CMD_GET_STATUS_ABORT 0x60 +#define KCS_CMD_WRITE_START 0x61 +#define KCS_CMD_WRITE_END 0x62 +#define KCS_CMD_READ_BYTE 0x68 + +#endif /* __IPMI_KCS_H__ */ -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-09-28 12:30 [PATCH 0/3] Add MCTP-over-KCS transport binding Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 1/3] ipmi: Move KCS headers to common include folder Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 2/3] ipmi: Create header with KCS interface defines Konstantin Aladyshev @ 2023-09-28 12:30 ` Konstantin Aladyshev 2023-09-28 22:58 ` kernel test robot 2023-09-29 11:08 ` Jonathan Cameron 2 siblings, 2 replies; 10+ messages in thread From: Konstantin Aladyshev @ 2023-09-28 12:30 UTC (permalink / raw) Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, aladyshev22, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev This change adds a MCTP KCS transport binding, as defined by the DMTF specificiation DSP0254 - "MCTP KCS Transport Binding". A MCTP protocol network device is created for each KCS channel found in the system. The interrupt code for the KCS state machine is based on the current IPMI KCS driver. Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> --- drivers/net/mctp/Kconfig | 8 + drivers/net/mctp/Makefile | 1 + drivers/net/mctp/mctp-kcs.c | 624 ++++++++++++++++++++++++++++++++++++ 3 files changed, 633 insertions(+) create mode 100644 drivers/net/mctp/mctp-kcs.c diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig index dc71657d9184..a37f7ba947c0 100644 --- a/drivers/net/mctp/Kconfig +++ b/drivers/net/mctp/Kconfig @@ -33,6 +33,14 @@ config MCTP_TRANSPORT_I2C from DMTF specification DSP0237. A MCTP protocol network device is created for each I2C bus that has been assigned a mctp-i2c device. +config MCTP_TRANSPORT_KCS + tristate "MCTP KCS transport" + depends on IPMI_KCS_BMC + help + Provides a driver to access MCTP devices over KCS transport, from + DMTF specification DSP0254. A MCTP protocol network device is + created for each KCS channel found in the system. + endmenu endif diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile index 1ca3e6028f77..885339a40f22 100644 --- a/drivers/net/mctp/Makefile +++ b/drivers/net/mctp/Makefile @@ -1,2 +1,3 @@ obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o +obj-$(CONFIG_MCTP_TRANSPORT_KCS) += mctp-kcs.o diff --git a/drivers/net/mctp/mctp-kcs.c b/drivers/net/mctp/mctp-kcs.c new file mode 100644 index 000000000000..8d8b77ad709c --- /dev/null +++ b/drivers/net/mctp/mctp-kcs.c @@ -0,0 +1,624 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * Management Component Transport Protocol (MCTP) KCS transport binding. + * This driver is an implementation of the DMTF specificiation + * "DSP0254 - Management Component Transport Protocol (MCTP) KCS Transport + * Binding", available at: + * + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0254_1.0.0.pdf + * + * This driver provides DSP0254-type MCTP-over-KCS transport using a Linux + * KCS client subsystem. + * + * Copyright (c) 2023 Konstantin Aladyshev <aladyshev22@gmail.com> + */ + +#include <linux/if_arp.h> +#include <linux/ipmi_kcs.h> +#include <linux/kcs_bmc_client.h> +#include <linux/mctp.h> +#include <linux/module.h> +#include <linux/mutex.h> +#include <linux/netdevice.h> +#include <net/mctp.h> +#include <net/mctpdevice.h> +#include <net/pkt_sched.h> + +#define MCTP_KCS_MTU 64 +#define KCS_MSG_BUFSIZ 1000 + +struct mctp_kcs { + struct list_head entry; + + /* protects rx & tx state machines */ + spinlock_t lock; + + struct kcs_bmc_client client; + struct net_device *netdev; + + enum kcs_ipmi_phases phase; + enum kcs_ipmi_errors error; + + int data_in_idx; + u8 *data_in; + + int data_out_idx; + int data_out_len; + u8 *data_out; + + struct work_struct rx_work; +}; + +struct mctp_kcs_header { + u8 netfn_lun; + u8 defining_body; + u8 len; +} __packed; + +struct mctp_kcs_trailer { + u8 pec; +} __packed; + +// According to SMBUS spec, the polynomial is: +// // C(x) = X^8 + X^2 + X^1 + 1, which is 0x107, +// // just ignore bit8 in definition. +#define MCTP_KCS_PACKET_ERROR_CODE_POLY 0x07 +// +#define MCTP_KCS_NETFN_LUN 0xb0 +#define DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP 0x01 + +static u8 generate_crc8(u8 polynomial, u8 crc_init_val, u8 *buffer, u32 len) +{ + u8 bit_index; + u32 buffer_index; + u8 crc = crc_init_val; + + buffer_index = 0; + while (buffer_index < len) { + crc = crc ^ *(buffer + buffer_index); + buffer_index++; + + for (bit_index = 0; bit_index < 8; bit_index++) { + if ((crc & 0x80) != 0) + crc = (crc << 1) ^ polynomial; + else + crc <<= 1; + } + } + return crc; +} + +static u8 generate_pec(u8 *buffer, u32 len) +{ + return generate_crc8(MCTP_KCS_PACKET_ERROR_CODE_POLY, 0, buffer, len); +} + +static int mctp_kcs_validate_data(struct mctp_kcs *mkcs, + struct mctp_kcs_header *hdr, int len) +{ + struct net_device *ndev = mkcs->netdev; + struct mctp_kcs_trailer *tlr; + u8 pec; + + if (hdr->netfn_lun != MCTP_KCS_NETFN_LUN) { + dev_err(mkcs->client.dev->dev, + "%s: KCS binding header error! netfn_lun = 0x%02x, but should be 0x%02x", + __func__, hdr->netfn_lun, MCTP_KCS_NETFN_LUN); + ndev->stats.rx_dropped++; + return -EINVAL; + } + if (hdr->defining_body != DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP) { + dev_err(mkcs->client.dev->dev, + "%s: KCS binding header error! defining_body = 0x%02x, but should be 0x%02x", + __func__, hdr->defining_body, + DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP); + ndev->stats.rx_dropped++; + return -EINVAL; + } + if (hdr->len != (len - sizeof(struct mctp_kcs_header) - + sizeof(struct mctp_kcs_trailer))) { + dev_err(mkcs->client.dev->dev, + "%s: KCS binding header error! len = 0x%02x, but should be 0x%02x", + __func__, hdr->len, + (len - sizeof(struct mctp_kcs_header) - + sizeof(struct mctp_kcs_trailer))); + ndev->stats.rx_length_errors++; + return -EINVAL; + } + + pec = generate_pec((u8 *)(hdr + 1), hdr->len); + tlr = (struct mctp_kcs_trailer *)((u8 *)(hdr + 1) + hdr->len); + if (pec != tlr->pec) { + dev_err(mkcs->client.dev->dev, + "%s: PEC error! Packet value=0x%02x, calculated value=0x%02x", + __func__, tlr->pec, pec); + ndev->stats.rx_crc_errors++; + return -EINVAL; + } + return 0; +} + +static void mctp_kcs_rx_work(struct work_struct *work) +{ + struct mctp_kcs *mkcs = container_of(work, struct mctp_kcs, rx_work); + struct mctp_skb_cb *cb; + struct sk_buff *skb; + unsigned long flags; + int rc; + int i; + struct mctp_kcs_header *kcs_header; + int data_len; + int data_in_idx; + + spin_lock_irqsave(&mkcs->lock, flags); + for (i = 0; i < (mkcs->data_in_idx); i++) + dev_dbg(mkcs->client.dev->dev, "%s: data_in[%d]=0x%02x", + __func__, i, mkcs->data_in[i]); + + data_len = mkcs->data_in_idx - sizeof(struct mctp_kcs_header) - + sizeof(struct mctp_kcs_trailer); + if (mkcs->phase != KCS_PHASE_WRITE_DONE) { + dev_err(mkcs->client.dev->dev, + "%s: error! Wrong KCS stage at the end of data read (phase=%d)", + __func__, mkcs->phase); + mkcs->netdev->stats.rx_dropped++; + spin_unlock_irqrestore(&mkcs->lock, flags); + return; + } + + mkcs->phase = KCS_PHASE_WAIT_READ; + data_in_idx = mkcs->data_in_idx; + mkcs->data_in_idx = 0; + + skb = netdev_alloc_skb(mkcs->netdev, data_len); + if (!skb) { + mkcs->netdev->stats.rx_dropped++; + spin_unlock_irqrestore(&mkcs->lock, flags); + return; + } + + kcs_header = (struct mctp_kcs_header *)mkcs->data_in; + rc = mctp_kcs_validate_data(mkcs, kcs_header, data_in_idx); + if (rc) { + dev_err(mkcs->client.dev->dev, + "%s: error! Binding validation failed", __func__); + dev_kfree_skb(skb); + spin_unlock_irqrestore(&mkcs->lock, flags); + return; + } + + skb->protocol = htons(ETH_P_MCTP); + skb_put_data(skb, mkcs->data_in + sizeof(struct mctp_kcs_header), + data_len); + spin_unlock_irqrestore(&mkcs->lock, flags); + skb_reset_network_header(skb); + + cb = __mctp_cb(skb); + cb->halen = 0; + + netif_rx(skb); + mkcs->netdev->stats.rx_packets++; + mkcs->netdev->stats.rx_bytes += data_len; +} + +static netdev_tx_t mctp_kcs_start_xmit(struct sk_buff *skb, + struct net_device *ndev) +{ + struct mctp_kcs_header *kcs_header; + unsigned long flags; + int i; + struct mctp_kcs *mkcs = netdev_priv(ndev); + + if (skb->len > MCTP_KCS_MTU) { + dev_err(&ndev->dev, "%s: error! skb len is bigger than MTU", + __func__); + ndev->stats.tx_dropped++; + goto out; + } + + spin_lock_irqsave(&mkcs->lock, flags); + if (mkcs->phase != KCS_PHASE_WAIT_READ) { + dev_err(&ndev->dev, + "%s: error! Wrong KCS stage at the start of data write (phase=%d)", + __func__, mkcs->phase); + dev_kfree_skb(skb); + spin_unlock_irqrestore(&mkcs->lock, flags); + return NETDEV_TX_BUSY; + } + + netif_stop_queue(ndev); + mkcs->phase = KCS_PHASE_READ; + kcs_header = (struct mctp_kcs_header *)mkcs->data_out; + kcs_header->netfn_lun = MCTP_KCS_NETFN_LUN; + kcs_header->defining_body = DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP; + kcs_header->len = skb->len; + skb_copy_bits(skb, 0, kcs_header + 1, skb->len); + mkcs->data_out[sizeof(struct mctp_kcs_header) + skb->len] = + generate_pec((u8 *)(kcs_header + 1), skb->len); + mkcs->data_out_idx = 1; + mkcs->data_out_len = skb->len + sizeof(struct mctp_kcs_header) + + sizeof(struct mctp_kcs_trailer); + + for (i = 0; i < (mkcs->data_out_len); i++) + dev_dbg(&ndev->dev, "%s: data_out[%d]=0x%02x", __func__, i, + mkcs->data_out[i]); + + // Write first data byte to initialize transmission + kcs_bmc_write_data(mkcs->client.dev, mkcs->data_out[0]); + + spin_unlock_irqrestore(&mkcs->lock, flags); +out: + dev_kfree_skb(skb); + return NETDEV_TX_OK; +} + +static inline void set_state(struct mctp_kcs *mkcs, u8 state) +{ + dev_dbg(mkcs->client.dev->dev, "%s: state=0x%02x", __func__, state); + kcs_bmc_update_status(mkcs->client.dev, KCS_STATUS_STATE_MASK, + KCS_STATUS_STATE(state)); +} + +static int mctp_kcs_ndo_open(struct net_device *ndev) +{ + struct mctp_kcs *mkcs; + + mkcs = netdev_priv(ndev); + dev_info(&ndev->dev, "Open MCTP over KCS channel %d", + mkcs->client.dev->channel); + return kcs_bmc_enable_device(mkcs->client.dev, &mkcs->client); +} + +static int mctp_kcs_ndo_stop(struct net_device *ndev) +{ + struct mctp_kcs *mkcs; + + mkcs = netdev_priv(ndev); + dev_info(&ndev->dev, "Stop MCTP over KCS channel %d", + mkcs->client.dev->channel); + mkcs->data_in_idx = 0; + mkcs->data_out_idx = 0; + mkcs->data_out_len = 0; + mkcs->phase = KCS_PHASE_IDLE; + set_state(mkcs, IDLE_STATE); + kcs_bmc_disable_device(mkcs->client.dev, &mkcs->client); + return 0; +} + +static const struct net_device_ops mctp_kcs_netdev_ops = { + .ndo_start_xmit = mctp_kcs_start_xmit, + .ndo_open = mctp_kcs_ndo_open, + .ndo_stop = mctp_kcs_ndo_stop, +}; + +static void mctp_kcs_setup(struct net_device *ndev) +{ + ndev->type = ARPHRD_MCTP; + + /* we limit at the fixed MTU, which is also the MCTP-standard + * baseline MTU, so is also our minimum + */ + ndev->mtu = MCTP_KCS_MTU; + ndev->max_mtu = MCTP_KCS_MTU; + ndev->min_mtu = MCTP_KCS_MTU; + + ndev->hard_header_len = 0; + ndev->addr_len = 0; + ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; + ndev->flags = IFF_NOARP; + ndev->netdev_ops = &mctp_kcs_netdev_ops; + ndev->needs_free_netdev = true; +} + +static void kcs_bmc_ipmi_force_abort(struct mctp_kcs *mkcs) +{ + dev_err(mkcs->client.dev->dev, + "Error! Force abort on KCS communication"); + set_state(mkcs, ERROR_STATE); + kcs_bmc_read_data(mkcs->client.dev); + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); + mkcs->phase = KCS_PHASE_ERROR; + mkcs->data_in_idx = 0; +} + +static void kcs_bmc_ipmi_handle_data(struct mctp_kcs *mkcs) +{ + struct kcs_bmc_device *dev; + u8 data; + + dev = mkcs->client.dev; + + switch (mkcs->phase) { + case KCS_PHASE_WRITE_START: + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_START", __func__); + mkcs->phase = KCS_PHASE_WRITE_DATA; + fallthrough; + + case KCS_PHASE_WRITE_DATA: + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_DATA", __func__); + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { + set_state(mkcs, WRITE_STATE); + kcs_bmc_write_data(dev, KCS_ZERO_DATA); + mkcs->data_in[mkcs->data_in_idx++] = + kcs_bmc_read_data(dev); + dev_dbg(dev->dev, + "%s: KCS_PHASE_WRITE_DATA: data_in[%d]=0x%02x", + __func__, mkcs->data_in_idx - 1, + mkcs->data_in[mkcs->data_in_idx - 1]); + } else { + kcs_bmc_ipmi_force_abort(mkcs); + mkcs->error = KCS_LENGTH_ERROR; + } + break; + + case KCS_PHASE_WRITE_END_CMD: + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_END_CMD", __func__); + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { + set_state(mkcs, READ_STATE); + mkcs->data_in[mkcs->data_in_idx++] = + kcs_bmc_read_data(dev); + dev_dbg(dev->dev, + "%s: KCS_PHASE_WRITE_END_CMD: data_in[%d]=0x%02x", + __func__, mkcs->data_in_idx - 1, + mkcs->data_in[mkcs->data_in_idx - 1]); + mkcs->phase = KCS_PHASE_WRITE_DONE; + schedule_work(&mkcs->rx_work); + } else { + kcs_bmc_ipmi_force_abort(mkcs); + mkcs->error = KCS_LENGTH_ERROR; + } + break; + + case KCS_PHASE_READ: + dev_dbg(dev->dev, + "%s: KCS_PHASE_READ, data_out_idx=%d, data_out_len=%d", + __func__, mkcs->data_out_idx, mkcs->data_out_len); + if (mkcs->data_out_idx == mkcs->data_out_len) + set_state(mkcs, IDLE_STATE); + + data = kcs_bmc_read_data(dev); + if (data != KCS_CMD_READ_BYTE) { + dev_dbg(dev->dev, + "%s: error! data is not equal to KCS_CMD_READ_BYTE", + __func__); + set_state(mkcs, ERROR_STATE); + kcs_bmc_write_data(dev, KCS_ZERO_DATA); + break; + } + + if (mkcs->data_out_idx == mkcs->data_out_len) { + kcs_bmc_write_data(dev, KCS_ZERO_DATA); + mkcs->netdev->stats.tx_bytes += mkcs->data_out_len; + mkcs->netdev->stats.tx_packets++; + mkcs->phase = KCS_PHASE_IDLE; + if (netif_queue_stopped(mkcs->netdev)) + netif_start_queue(mkcs->netdev); + break; + } + + dev_dbg(dev->dev, "%s: KCS_PHASE_READ: data_out[%d]=0x%02x", + __func__, mkcs->data_out_idx, + mkcs->data_out[mkcs->data_out_idx]); + kcs_bmc_write_data(dev, mkcs->data_out[mkcs->data_out_idx++]); + break; + + case KCS_PHASE_ABORT_ERROR1: + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR1", __func__); + set_state(mkcs, READ_STATE); + kcs_bmc_read_data(dev); + kcs_bmc_write_data(dev, mkcs->error); + mkcs->phase = KCS_PHASE_ABORT_ERROR2; + break; + + case KCS_PHASE_ABORT_ERROR2: + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR2", __func__); + set_state(mkcs, IDLE_STATE); + kcs_bmc_read_data(dev); + kcs_bmc_write_data(dev, KCS_ZERO_DATA); + mkcs->phase = KCS_PHASE_IDLE; + break; + + default: + dev_dbg(dev->dev, "%s: unknown KCS phase", __func__); + kcs_bmc_ipmi_force_abort(mkcs); + break; + } +} + +static void kcs_bmc_ipmi_handle_cmd(struct mctp_kcs *mkcs) +{ + struct kcs_bmc_device *dev; + u8 cmd; + + dev = mkcs->client.dev; + + set_state(mkcs, WRITE_STATE); + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); + + cmd = kcs_bmc_read_data(mkcs->client.dev); + switch (cmd) { + case KCS_CMD_WRITE_START: + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_START", __func__); + mkcs->phase = KCS_PHASE_WRITE_START; + mkcs->error = KCS_NO_ERROR; + mkcs->data_in_idx = 0; + break; + + case KCS_CMD_WRITE_END: + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_END", __func__); + if (mkcs->phase != KCS_PHASE_WRITE_DATA) { + kcs_bmc_ipmi_force_abort(mkcs); + break; + } + mkcs->phase = KCS_PHASE_WRITE_END_CMD; + break; + + case KCS_CMD_GET_STATUS_ABORT: + dev_dbg(dev->dev, "%s: KCS_CMD_GET_STATUS_ABORT", __func__); + if (mkcs->error == KCS_NO_ERROR) + mkcs->error = KCS_ABORTED_BY_COMMAND; + + mkcs->phase = KCS_PHASE_ABORT_ERROR1; + mkcs->data_in_idx = 0; + break; + + default: + dev_dbg(dev->dev, "%s: unknown KCS command", __func__); + kcs_bmc_ipmi_force_abort(mkcs); + mkcs->error = KCS_ILLEGAL_CONTROL_CODE; + break; + } +} + +static inline struct mctp_kcs *client_to_mctp_kcs(struct kcs_bmc_client *client) +{ + return container_of(client, struct mctp_kcs, client); +} + +static irqreturn_t kcs_bmc_mctp_event(struct kcs_bmc_client *client) +{ + struct mctp_kcs *mkcs; + u8 status; + int ret; + + mkcs = client_to_mctp_kcs(client); + if (!mkcs) { + dev_err(client->dev->dev, + "%s: error! can't find mctp_kcs from KCS client", + __func__); + return IRQ_NONE; + } + + spin_lock(&mkcs->lock); + + status = kcs_bmc_read_status(client->dev); + if (status & KCS_STATUS_IBF) { + if (status & KCS_STATUS_CMD_DAT) + kcs_bmc_ipmi_handle_cmd(mkcs); + else + kcs_bmc_ipmi_handle_data(mkcs); + + ret = IRQ_HANDLED; + } else { + ret = IRQ_NONE; + } + + spin_unlock(&mkcs->lock); + + return ret; +} + +static const struct kcs_bmc_client_ops kcs_bmc_mctp_client_ops = { + .event = kcs_bmc_mctp_event, +}; + +static DEFINE_SPINLOCK(kcs_bmc_mctp_instances_lock); +static LIST_HEAD(kcs_bmc_mctp_instances); + +static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) +{ + struct mctp_kcs *mkcs; + struct net_device *ndev; + char name[32]; + int rc; + + snprintf(name, sizeof(name), "mctpkcs%d", kcs_bmc->channel); + + ndev = alloc_netdev(sizeof(*mkcs), name, NET_NAME_ENUM, mctp_kcs_setup); + if (!ndev) { + dev_err(kcs_bmc->dev, + "alloc_netdev failed for KCS channel %d\n", + kcs_bmc->channel); + rc = -ENOMEM; + goto err; + } + + mkcs = netdev_priv(ndev); + mkcs->netdev = ndev; + mkcs->client.dev = kcs_bmc; + mkcs->client.ops = &kcs_bmc_mctp_client_ops; + mkcs->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); + mkcs->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); + if (!mkcs->data_in || !mkcs->data_out) { + dev_err(kcs_bmc->dev, + "failed to allocate data buffers for KCS channel %d\n", + kcs_bmc->channel); + rc = -ENOMEM; + goto free_netdev; + } + + INIT_WORK(&mkcs->rx_work, mctp_kcs_rx_work); + + rc = register_netdev(ndev); + if (rc) + goto free_netdev; + + spin_lock_irq(&kcs_bmc_mctp_instances_lock); + list_add(&mkcs->entry, &kcs_bmc_mctp_instances); + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); + + dev_info(kcs_bmc->dev, "Add MCTP client for the KCS channel %d", + kcs_bmc->channel); + return 0; + +free_netdev: + if (ndev) + free_netdev(ndev); + +err: + return rc; +} + +static int kcs_bmc_mctp_remove_device(struct kcs_bmc_device *kcs_bmc) +{ + struct mctp_kcs *mkcs = NULL, *pos; + + dev_info(kcs_bmc->dev, "Remove MCTP client for the KCS channel %d", + kcs_bmc->channel); + spin_lock_irq(&kcs_bmc_mctp_instances_lock); + list_for_each_entry(pos, &kcs_bmc_mctp_instances, entry) { + if (pos->client.dev == kcs_bmc) { + mkcs = pos; + list_del(&pos->entry); + break; + } + } + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); + + if (!mkcs) + return -ENODEV; + + unregister_netdev(mkcs->netdev); + free_netdev(mkcs->netdev); + kcs_bmc_disable_device(mkcs->client.dev, &mkcs->client); + devm_kfree(kcs_bmc->dev, mkcs->data_in); + devm_kfree(kcs_bmc->dev, mkcs->data_out); + return 0; +} + +static const struct kcs_bmc_driver_ops kcs_bmc_mctp_driver_ops = { + .add_device = kcs_bmc_mctp_add_device, + .remove_device = kcs_bmc_mctp_remove_device, +}; + +static struct kcs_bmc_driver kcs_bmc_mctp_driver = { + .ops = &kcs_bmc_mctp_driver_ops, +}; + +static int __init mctp_kcs_init(void) +{ + kcs_bmc_register_driver(&kcs_bmc_mctp_driver); + return 0; +} + +static void __exit mctp_kcs_exit(void) +{ + kcs_bmc_unregister_driver(&kcs_bmc_mctp_driver); +} + +module_init(mctp_kcs_init); +module_exit(mctp_kcs_exit); + +MODULE_LICENSE("GPL"); +MODULE_AUTHOR("Konstantin Aladyshev <aladyshev22@gmail.com>"); +MODULE_DESCRIPTION("MCTP KCS transport"); -- 2.25.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-09-28 12:30 ` [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding Konstantin Aladyshev @ 2023-09-28 22:58 ` kernel test robot 2023-09-29 11:08 ` Jonathan Cameron 1 sibling, 0 replies; 10+ messages in thread From: kernel test robot @ 2023-09-28 22:58 UTC (permalink / raw) To: Konstantin Aladyshev Cc: oe-kbuild-all, minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, aladyshev22, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev Hi Konstantin, kernel test robot noticed the following build warnings: [auto build test WARNING on cminyard-ipmi/for-next] [also build test WARNING on linus/master v6.6-rc3 next-20230928] [If your patch is applied to the wrong git tree, kindly drop us a note. And when submitting patch, we suggest to use '--base' as documented in https://git-scm.com/docs/git-format-patch#_base_tree_information] url: https://github.com/intel-lab-lkp/linux/commits/Konstantin-Aladyshev/ipmi-Move-KCS-headers-to-common-include-folder/20230928-203248 base: https://github.com/cminyard/linux-ipmi for-next patch link: https://lore.kernel.org/r/20230928123009.2913-4-aladyshev22%40gmail.com patch subject: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding config: sparc-allyesconfig (https://download.01.org/0day-ci/archive/20230929/202309290613.qxRTI9f7-lkp@intel.com/config) compiler: sparc64-linux-gcc (GCC) 13.2.0 reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20230929/202309290613.qxRTI9f7-lkp@intel.com/reproduce) If you fix the issue in a separate patch/commit (i.e. not just a new version of the same patch/commit), kindly add following tags | Reported-by: kernel test robot <lkp@intel.com> | Closes: https://lore.kernel.org/oe-kbuild-all/202309290613.qxRTI9f7-lkp@intel.com/ All warnings (new ones prefixed by >>): In file included from include/linux/device.h:15, from include/linux/dma-mapping.h:8, from include/linux/skbuff.h:28, from include/linux/if_arp.h:22, from drivers/net/mctp/mctp-kcs.c:16: drivers/net/mctp/mctp-kcs.c: In function 'mctp_kcs_validate_data': >> drivers/net/mctp/mctp-kcs.c:121:25: warning: format '%x' expects argument of type 'unsigned int', but argument 5 has type 'long unsigned int' [-Wformat=] 121 | "%s: KCS binding header error! len = 0x%02x, but should be 0x%02x", | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ include/linux/dev_printk.h:110:30: note: in definition of macro 'dev_printk_index_wrap' 110 | _p_func(dev, fmt, ##__VA_ARGS__); \ | ^~~ include/linux/dev_printk.h:144:56: note: in expansion of macro 'dev_fmt' 144 | dev_printk_index_wrap(_dev_err, KERN_ERR, dev, dev_fmt(fmt), ##__VA_ARGS__) | ^~~~~~~ drivers/net/mctp/mctp-kcs.c:120:17: note: in expansion of macro 'dev_err' 120 | dev_err(mkcs->client.dev->dev, | ^~~~~~~ drivers/net/mctp/mctp-kcs.c:121:89: note: format string is defined here 121 | "%s: KCS binding header error! len = 0x%02x, but should be 0x%02x", | ~~~^ | | | unsigned int | %02lx vim +121 drivers/net/mctp/mctp-kcs.c 95 96 static int mctp_kcs_validate_data(struct mctp_kcs *mkcs, 97 struct mctp_kcs_header *hdr, int len) 98 { 99 struct net_device *ndev = mkcs->netdev; 100 struct mctp_kcs_trailer *tlr; 101 u8 pec; 102 103 if (hdr->netfn_lun != MCTP_KCS_NETFN_LUN) { 104 dev_err(mkcs->client.dev->dev, 105 "%s: KCS binding header error! netfn_lun = 0x%02x, but should be 0x%02x", 106 __func__, hdr->netfn_lun, MCTP_KCS_NETFN_LUN); 107 ndev->stats.rx_dropped++; 108 return -EINVAL; 109 } 110 if (hdr->defining_body != DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP) { 111 dev_err(mkcs->client.dev->dev, 112 "%s: KCS binding header error! defining_body = 0x%02x, but should be 0x%02x", 113 __func__, hdr->defining_body, 114 DEFINING_BODY_DMTF_PRE_OS_WORKING_GROUP); 115 ndev->stats.rx_dropped++; 116 return -EINVAL; 117 } 118 if (hdr->len != (len - sizeof(struct mctp_kcs_header) - 119 sizeof(struct mctp_kcs_trailer))) { 120 dev_err(mkcs->client.dev->dev, > 121 "%s: KCS binding header error! len = 0x%02x, but should be 0x%02x", 122 __func__, hdr->len, 123 (len - sizeof(struct mctp_kcs_header) - 124 sizeof(struct mctp_kcs_trailer))); 125 ndev->stats.rx_length_errors++; 126 return -EINVAL; 127 } 128 129 pec = generate_pec((u8 *)(hdr + 1), hdr->len); 130 tlr = (struct mctp_kcs_trailer *)((u8 *)(hdr + 1) + hdr->len); 131 if (pec != tlr->pec) { 132 dev_err(mkcs->client.dev->dev, 133 "%s: PEC error! Packet value=0x%02x, calculated value=0x%02x", 134 __func__, tlr->pec, pec); 135 ndev->stats.rx_crc_errors++; 136 return -EINVAL; 137 } 138 return 0; 139 } 140 -- 0-DAY CI Kernel Test Service https://github.com/intel/lkp-tests/wiki ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-09-28 12:30 ` [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding Konstantin Aladyshev 2023-09-28 22:58 ` kernel test robot @ 2023-09-29 11:08 ` Jonathan Cameron 2023-10-02 14:41 ` Konstantin Aladyshev 2023-10-03 7:22 ` Andrew Jeffery 1 sibling, 2 replies; 10+ messages in thread From: Jonathan Cameron @ 2023-09-29 11:08 UTC (permalink / raw) To: Konstantin Aladyshev Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev On Thu, 28 Sep 2023 15:30:09 +0300 Konstantin Aladyshev <aladyshev22@gmail.com> wrote: > This change adds a MCTP KCS transport binding, as defined by the DMTF > specificiation DSP0254 - "MCTP KCS Transport Binding". > A MCTP protocol network device is created for each KCS channel found in > the system. > The interrupt code for the KCS state machine is based on the current > IPMI KCS driver. > > Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> Drive by review as I was curious and might as well comment whilst reading. Some comments seem to equally apply to other kcs drivers so maybe I'm missing something... Jonathan > --- > drivers/net/mctp/Kconfig | 8 + > drivers/net/mctp/Makefile | 1 + > drivers/net/mctp/mctp-kcs.c | 624 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 633 insertions(+) > create mode 100644 drivers/net/mctp/mctp-kcs.c > > diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig > index dc71657d9184..a37f7ba947c0 100644 > --- a/drivers/net/mctp/Kconfig > +++ b/drivers/net/mctp/Kconfig > @@ -33,6 +33,14 @@ config MCTP_TRANSPORT_I2C > from DMTF specification DSP0237. A MCTP protocol network device is > created for each I2C bus that has been assigned a mctp-i2c device. > > +config MCTP_TRANSPORT_KCS > + tristate "MCTP KCS transport" > + depends on IPMI_KCS_BMC > + help > + Provides a driver to access MCTP devices over KCS transport, from > + DMTF specification DSP0254. A MCTP protocol network device is > + created for each KCS channel found in the system. > + > endmenu > > endif > diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile > index 1ca3e6028f77..885339a40f22 100644 > --- a/drivers/net/mctp/Makefile > +++ b/drivers/net/mctp/Makefile > @@ -1,2 +1,3 @@ > obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o > obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o > +obj-$(CONFIG_MCTP_TRANSPORT_KCS) += mctp-kcs.o > diff --git a/drivers/net/mctp/mctp-kcs.c b/drivers/net/mctp/mctp-kcs.c > new file mode 100644 > index 000000000000..8d8b77ad709c > --- /dev/null > +++ b/drivers/net/mctp/mctp-kcs.c > @@ -0,0 +1,624 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * Management Component Transport Protocol (MCTP) KCS transport binding. > + * This driver is an implementation of the DMTF specificiation > + * "DSP0254 - Management Component Transport Protocol (MCTP) KCS Transport > + * Binding", available at: > + * > + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0254_1.0.0.pdf > + * > + * This driver provides DSP0254-type MCTP-over-KCS transport using a Linux > + * KCS client subsystem. > + * > + * Copyright (c) 2023 Konstantin Aladyshev <aladyshev22@gmail.com> > + */ > + > +#include <linux/if_arp.h> > +#include <linux/ipmi_kcs.h> > +#include <linux/kcs_bmc_client.h> > +#include <linux/mctp.h> > +#include <linux/module.h> > +#include <linux/mutex.h> Check these. There aren't any mutex's in here that I noticed... > +#include <linux/netdevice.h> > +#include <net/mctp.h> > +#include <net/mctpdevice.h> > +#include <net/pkt_sched.h> > +// According to SMBUS spec, the polynomial is: > +// // C(x) = X^8 + X^2 + X^1 + 1, which is 0x107, > +// // just ignore bit8 in definition. I'm confused by this part. Is this calling out a specification issue, or something that the relevant specs make clear? If the second, add a reference. If it's a specification issue that's fine, but are we sure it will get resolved hwo you've done it here? > +#define MCTP_KCS_PACKET_ERROR_CODE_POLY 0x07 > +// > + > +static void mctp_kcs_rx_work(struct work_struct *work) > +{ > + struct mctp_kcs *mkcs = container_of(work, struct mctp_kcs, rx_work); > + struct mctp_skb_cb *cb; > + struct sk_buff *skb; > + unsigned long flags; > + int rc; > + int i; > + struct mctp_kcs_header *kcs_header; > + int data_len; > + int data_in_idx; > + > + spin_lock_irqsave(&mkcs->lock, flags); > + for (i = 0; i < (mkcs->data_in_idx); i++) > + dev_dbg(mkcs->client.dev->dev, "%s: data_in[%d]=0x%02x", > + __func__, i, mkcs->data_in[i]); > + > + data_len = mkcs->data_in_idx - sizeof(struct mctp_kcs_header) - > + sizeof(struct mctp_kcs_trailer); > + if (mkcs->phase != KCS_PHASE_WRITE_DONE) { > + dev_err(mkcs->client.dev->dev, > + "%s: error! Wrong KCS stage at the end of data read (phase=%d)", > + __func__, mkcs->phase); > + mkcs->netdev->stats.rx_dropped++; > + spin_unlock_irqrestore(&mkcs->lock, flags); > + return; > + } > + > + mkcs->phase = KCS_PHASE_WAIT_READ; > + data_in_idx = mkcs->data_in_idx; > + mkcs->data_in_idx = 0; > + > + skb = netdev_alloc_skb(mkcs->netdev, data_len); > + if (!skb) { > + mkcs->netdev->stats.rx_dropped++; > + spin_unlock_irqrestore(&mkcs->lock, flags); I'd use a sequence of gotos and labels to deal with the different cleanup. > + return; > + } > + > + kcs_header = (struct mctp_kcs_header *)mkcs->data_in; > + rc = mctp_kcs_validate_data(mkcs, kcs_header, data_in_idx); > + if (rc) { > + dev_err(mkcs->client.dev->dev, > + "%s: error! Binding validation failed", __func__); > + dev_kfree_skb(skb); > + spin_unlock_irqrestore(&mkcs->lock, flags); > + return; > + } > + > + skb->protocol = htons(ETH_P_MCTP); > + skb_put_data(skb, mkcs->data_in + sizeof(struct mctp_kcs_header), > + data_len); > + spin_unlock_irqrestore(&mkcs->lock, flags); > + skb_reset_network_header(skb); > + > + cb = __mctp_cb(skb); > + cb->halen = 0; > + > + netif_rx(skb); > + mkcs->netdev->stats.rx_packets++; > + mkcs->netdev->stats.rx_bytes += data_len; > +} > + > +static void mctp_kcs_setup(struct net_device *ndev) > +{ > + ndev->type = ARPHRD_MCTP; > + > + /* we limit at the fixed MTU, which is also the MCTP-standard > + * baseline MTU, so is also our minimum > + */ > + ndev->mtu = MCTP_KCS_MTU; > + ndev->max_mtu = MCTP_KCS_MTU; > + ndev->min_mtu = MCTP_KCS_MTU; > + > + ndev->hard_header_len = 0; > + ndev->addr_len = 0; > + ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; > + ndev->flags = IFF_NOARP; > + ndev->netdev_ops = &mctp_kcs_netdev_ops; > + ndev->needs_free_netdev = true; > +} > + > +static void kcs_bmc_ipmi_force_abort(struct mctp_kcs *mkcs) > +{ > + dev_err(mkcs->client.dev->dev, > + "Error! Force abort on KCS communication"); > + set_state(mkcs, ERROR_STATE); > + kcs_bmc_read_data(mkcs->client.dev); > + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); > + mkcs->phase = KCS_PHASE_ERROR; > + mkcs->data_in_idx = 0; > +} > + > +static void kcs_bmc_ipmi_handle_data(struct mctp_kcs *mkcs) > +{ > + struct kcs_bmc_device *dev; > + u8 data; > + > + dev = mkcs->client.dev; As below, I'd put this on the line defining the local variable and rename it. > + > + switch (mkcs->phase) { > + case KCS_PHASE_WRITE_START: > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_START", __func__); > + mkcs->phase = KCS_PHASE_WRITE_DATA; > + fallthrough; > + > + case KCS_PHASE_WRITE_DATA: > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_DATA", __func__); > + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { > + set_state(mkcs, WRITE_STATE); > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > + mkcs->data_in[mkcs->data_in_idx++] = > + kcs_bmc_read_data(dev); > + dev_dbg(dev->dev, > + "%s: KCS_PHASE_WRITE_DATA: data_in[%d]=0x%02x", > + __func__, mkcs->data_in_idx - 1, > + mkcs->data_in[mkcs->data_in_idx - 1]); > + } else { > + kcs_bmc_ipmi_force_abort(mkcs); > + mkcs->error = KCS_LENGTH_ERROR; > + } > + break; > + > + case KCS_PHASE_WRITE_END_CMD: > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_END_CMD", __func__); > + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { > + set_state(mkcs, READ_STATE); > + mkcs->data_in[mkcs->data_in_idx++] = > + kcs_bmc_read_data(dev); > + dev_dbg(dev->dev, > + "%s: KCS_PHASE_WRITE_END_CMD: data_in[%d]=0x%02x", > + __func__, mkcs->data_in_idx - 1, > + mkcs->data_in[mkcs->data_in_idx - 1]); > + mkcs->phase = KCS_PHASE_WRITE_DONE; > + schedule_work(&mkcs->rx_work); > + } else { > + kcs_bmc_ipmi_force_abort(mkcs); > + mkcs->error = KCS_LENGTH_ERROR; > + } > + break; > + > + case KCS_PHASE_READ: > + dev_dbg(dev->dev, > + "%s: KCS_PHASE_READ, data_out_idx=%d, data_out_len=%d", > + __func__, mkcs->data_out_idx, mkcs->data_out_len); > + if (mkcs->data_out_idx == mkcs->data_out_len) > + set_state(mkcs, IDLE_STATE); > + > + data = kcs_bmc_read_data(dev); > + if (data != KCS_CMD_READ_BYTE) { > + dev_dbg(dev->dev, > + "%s: error! data is not equal to KCS_CMD_READ_BYTE", > + __func__); > + set_state(mkcs, ERROR_STATE); > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > + break; > + } > + > + if (mkcs->data_out_idx == mkcs->data_out_len) { > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > + mkcs->netdev->stats.tx_bytes += mkcs->data_out_len; > + mkcs->netdev->stats.tx_packets++; > + mkcs->phase = KCS_PHASE_IDLE; > + if (netif_queue_stopped(mkcs->netdev)) > + netif_start_queue(mkcs->netdev); > + break; > + } > + > + dev_dbg(dev->dev, "%s: KCS_PHASE_READ: data_out[%d]=0x%02x", > + __func__, mkcs->data_out_idx, > + mkcs->data_out[mkcs->data_out_idx]); > + kcs_bmc_write_data(dev, mkcs->data_out[mkcs->data_out_idx++]); > + break; > + > + case KCS_PHASE_ABORT_ERROR1: > + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR1", __func__); > + set_state(mkcs, READ_STATE); > + kcs_bmc_read_data(dev); > + kcs_bmc_write_data(dev, mkcs->error); > + mkcs->phase = KCS_PHASE_ABORT_ERROR2; > + break; > + > + case KCS_PHASE_ABORT_ERROR2: > + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR2", __func__); > + set_state(mkcs, IDLE_STATE); > + kcs_bmc_read_data(dev); > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > + mkcs->phase = KCS_PHASE_IDLE; > + break; > + > + default: > + dev_dbg(dev->dev, "%s: unknown KCS phase", __func__); > + kcs_bmc_ipmi_force_abort(mkcs); > + break; > + } > +} > + > +static void kcs_bmc_ipmi_handle_cmd(struct mctp_kcs *mkcs) > +{ > + struct kcs_bmc_device *dev; > + u8 cmd; > + > + dev = mkcs->client.dev; Might as well save a few lines as it doesn't hurt readability. struct kcs_bmc_device *dev = mkcs->client.dev; > + > + set_state(mkcs, WRITE_STATE); > + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); > + > + cmd = kcs_bmc_read_data(mkcs->client.dev); > + switch (cmd) { Local variable doesn't add anything really and you should use your handy 'dev'. Maybe rename dev, as most readers will assume it's a struct device and get confused (briefly) at the dev->dev bits in here. switch (kcs_bmc_read_data(dev)) { > + case KCS_CMD_WRITE_START: > + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_START", __func__); > + mkcs->phase = KCS_PHASE_WRITE_START; > + mkcs->error = KCS_NO_ERROR; > + mkcs->data_in_idx = 0; > + break; > + > + case KCS_CMD_WRITE_END: > + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_END", __func__); > + if (mkcs->phase != KCS_PHASE_WRITE_DATA) { > + kcs_bmc_ipmi_force_abort(mkcs); > + break; > + } > + mkcs->phase = KCS_PHASE_WRITE_END_CMD; > + break; > + > + case KCS_CMD_GET_STATUS_ABORT: > + dev_dbg(dev->dev, "%s: KCS_CMD_GET_STATUS_ABORT", __func__); > + if (mkcs->error == KCS_NO_ERROR) > + mkcs->error = KCS_ABORTED_BY_COMMAND; > + > + mkcs->phase = KCS_PHASE_ABORT_ERROR1; > + mkcs->data_in_idx = 0; > + break; > + > + default: > + dev_dbg(dev->dev, "%s: unknown KCS command", __func__); > + kcs_bmc_ipmi_force_abort(mkcs); > + mkcs->error = KCS_ILLEGAL_CONTROL_CODE; > + break; > + } > +} > + > +static inline struct mctp_kcs *client_to_mctp_kcs(struct kcs_bmc_client *client) > +{ > + return container_of(client, struct mctp_kcs, client); > +} > + > +static irqreturn_t kcs_bmc_mctp_event(struct kcs_bmc_client *client) > +{ > + struct mctp_kcs *mkcs; > + u8 status; > + int ret; > + > + mkcs = client_to_mctp_kcs(client); > + if (!mkcs) { > + dev_err(client->dev->dev, > + "%s: error! can't find mctp_kcs from KCS client", > + __func__); > + return IRQ_NONE; > + } > + > + spin_lock(&mkcs->lock); > + > + status = kcs_bmc_read_status(client->dev); > + if (status & KCS_STATUS_IBF) { > + if (status & KCS_STATUS_CMD_DAT) > + kcs_bmc_ipmi_handle_cmd(mkcs); > + else > + kcs_bmc_ipmi_handle_data(mkcs); > + > + ret = IRQ_HANDLED; > + } else { > + ret = IRQ_NONE; > + } > + > + spin_unlock(&mkcs->lock); > + > + return ret; > +} > + > +static const struct kcs_bmc_client_ops kcs_bmc_mctp_client_ops = { > + .event = kcs_bmc_mctp_event, > +}; > + > +static DEFINE_SPINLOCK(kcs_bmc_mctp_instances_lock); > +static LIST_HEAD(kcs_bmc_mctp_instances); As mentioned below, this seems to be only used to find some data again in remove. Lots of cleaner ways to do that than a list in the driver. I'd explore the alternatives. > + > +static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) > +{ > + struct mctp_kcs *mkcs; > + struct net_device *ndev; > + char name[32]; > + int rc; > + > + snprintf(name, sizeof(name), "mctpkcs%d", kcs_bmc->channel); > + > + ndev = alloc_netdev(sizeof(*mkcs), name, NET_NAME_ENUM, mctp_kcs_setup); Interesting that there is an explicit devm_register_netdev() but not one for this simple allocation case (there is one for the ethernet specific version). Never mind, we have devm_add_action_or_reset() for that. Just create a small wrapper for free_netdev() (which will look like devm_free_netdev() in net/devres.c but that's local to that file) and add rc = devm_add_action_or_reset(&kcs_bmc->dev, wrapper_for_free_netdev(), ndev); if (rc) return rc; > + if (!ndev) { > + dev_err(kcs_bmc->dev, > + "alloc_netdev failed for KCS channel %d\n", > + kcs_bmc->channel); No idea if the kcs subsystem handles deferred probing right, but in general anything called just in 'probe' routines can use dev_err_probe() to pretty print errors and also register any deferred cases with the logging stuff that lets you find out why they were deferred. > + rc = -ENOMEM; > + goto err; In general I find it easier to follow code that only uses a goto if there is shared cleanup to do. return -ENOMEM; and for this path I don't need to read further. > + } > + > + mkcs = netdev_priv(ndev); > + mkcs->netdev = ndev; > + mkcs->client.dev = kcs_bmc; > + mkcs->client.ops = &kcs_bmc_mctp_client_ops; > + mkcs->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > + mkcs->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); You should not be mixing device manged cleanup and manual cleanup. Rule of thumb is don't call any devm_ functions in a 'probe / add' type routine after you pass the first element that requires manual cleanup. Otherwise you get horrible race conditions or if not that, just code that is hard to check for them. > + if (!mkcs->data_in || !mkcs->data_out) { Check these independently. It costs a few extra lines but give more explicit messages and keeps ordering etc simpler. > + dev_err(kcs_bmc->dev, > + "failed to allocate data buffers for KCS channel %d\n", > + kcs_bmc->channel); > + rc = -ENOMEM; > + goto free_netdev; > + } > + > + INIT_WORK(&mkcs->rx_work, mctp_kcs_rx_work); > + > + rc = register_netdev(ndev); after change above, devm_register_netdev() > + if (rc) > + goto free_netdev; > + > + spin_lock_irq(&kcs_bmc_mctp_instances_lock); > + list_add(&mkcs->entry, &kcs_bmc_mctp_instances); Add a callback and devm_add_action_or_reset() to unwind this as well. > + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); > + > + dev_info(kcs_bmc->dev, "Add MCTP client for the KCS channel %d", > + kcs_bmc->channel); > + return 0; > + > +free_netdev: > + if (ndev) How do you get here with ndev not set? If there is a path, that's probably a bug. > + free_netdev(ndev); > + > +err: > + return rc; > +} > + > +static int kcs_bmc_mctp_remove_device(struct kcs_bmc_device *kcs_bmc) > +{ > + struct mctp_kcs *mkcs = NULL, *pos; > + > + dev_info(kcs_bmc->dev, "Remove MCTP client for the KCS channel %d", > + kcs_bmc->channel); > + spin_lock_irq(&kcs_bmc_mctp_instances_lock); > + list_for_each_entry(pos, &kcs_bmc_mctp_instances, entry) { > + if (pos->client.dev == kcs_bmc) { > + mkcs = pos; > + list_del(&pos->entry); > + break; I don't know the kcs stuff at all but these seems 'unusual'. Can't you stash device_set_drvdata(kcs_bmc->dev) or does it just match the structure containing the client pointed to by kcs_bmc_device? If so use something like container_of(kcs_bmc->client, struct mctp_kcs, client); Ah. You already have a function for that. Why not use that here? There isn't normally a reason for a driver to maintain an additional list like this. > + } > + } > + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); > + > + if (!mkcs) > + return -ENODEV; > + > + unregister_netdev(mkcs->netdev); > + free_netdev(mkcs->netdev); This stuff should be opposite order of add above, or leave it to devm to clean up. > + kcs_bmc_disable_device(mkcs->client.dev, &mkcs->client); This doesn't match with stuff in add - so I'd like a comment to explain why it is here. Also needs a comment on the ordering. Perhaps this is why you can't use devm for all the above, in which case I'd use it nowhere in this driver. I'm also confused on relationship between mks->client.dev and kcs_bmc (I'm fairly sure they are the same, so just use kcs_bmc here). > + devm_kfree(kcs_bmc->dev, mkcs->data_in); > + devm_kfree(kcs_bmc->dev, mkcs->data_out); Alarm bells occur whenever an explicit devm_kfree turns up in except in complex corner cases. Please look at how devm based resource management works. These should not be here. Also, remove_device should either do things in the opposite order to add_device, or it should have comments saying why not! > + return 0; > +} > + > +static const struct kcs_bmc_driver_ops kcs_bmc_mctp_driver_ops = { > + .add_device = kcs_bmc_mctp_add_device, > + .remove_device = kcs_bmc_mctp_remove_device, > +}; > + > +static struct kcs_bmc_driver kcs_bmc_mctp_driver = { > + .ops = &kcs_bmc_mctp_driver_ops, > +}; > + > +static int __init mctp_kcs_init(void) > +{ > + kcs_bmc_register_driver(&kcs_bmc_mctp_driver); > + return 0; > +} > + > +static void __exit mctp_kcs_exit(void) > +{ > + kcs_bmc_unregister_driver(&kcs_bmc_mctp_driver); > +} Hmm. So kcs is a very small subsystem hence no one has done the usual module_kcs_driver() wrapper (see something like module_i2c_driver) for an example. You can just use the underlying macro directly though to get rid of most of this boilerplate. module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, kcs_bmc_uregister_driver); > + > +module_init(mctp_kcs_init); > +module_exit(mctp_kcs_exit); > + > +MODULE_LICENSE("GPL"); > +MODULE_AUTHOR("Konstantin Aladyshev <aladyshev22@gmail.com>"); > +MODULE_DESCRIPTION("MCTP KCS transport"); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-09-29 11:08 ` Jonathan Cameron @ 2023-10-02 14:41 ` Konstantin Aladyshev 2023-10-03 16:17 ` Jonathan Cameron 2023-10-03 7:22 ` Andrew Jeffery 1 sibling, 1 reply; 10+ messages in thread From: Konstantin Aladyshev @ 2023-10-02 14:41 UTC (permalink / raw) To: Jonathan Cameron Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev Thanks for the review! I've corrected many things from your comments and have sent the V2 patch. I'm not sure about the LIST thing and all the devres management. I've written the KCS handling the same way it is done in the standard IPMI KCS driver (https://github.com/torvalds/linux/blob/master/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c) Not sure if we need to do any different here. Please see detailed response below: On Fri, Sep 29, 2023 at 2:08 PM Jonathan Cameron <Jonathan.Cameron@huawei.com> wrote: > > On Thu, 28 Sep 2023 15:30:09 +0300 > Konstantin Aladyshev <aladyshev22@gmail.com> wrote: > > > This change adds a MCTP KCS transport binding, as defined by the DMTF > > specificiation DSP0254 - "MCTP KCS Transport Binding". > > A MCTP protocol network device is created for each KCS channel found in > > the system. > > The interrupt code for the KCS state machine is based on the current > > IPMI KCS driver. > > > > Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> > > Drive by review as I was curious and might as well comment whilst reading. > Some comments seem to equally apply to other kcs drivers so maybe I'm > missing something... > > Jonathan > > > > --- > > drivers/net/mctp/Kconfig | 8 + > > drivers/net/mctp/Makefile | 1 + > > drivers/net/mctp/mctp-kcs.c | 624 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 633 insertions(+) > > create mode 100644 drivers/net/mctp/mctp-kcs.c > > > > diff --git a/drivers/net/mctp/Kconfig b/drivers/net/mctp/Kconfig > > index dc71657d9184..a37f7ba947c0 100644 > > --- a/drivers/net/mctp/Kconfig > > +++ b/drivers/net/mctp/Kconfig > > @@ -33,6 +33,14 @@ config MCTP_TRANSPORT_I2C > > from DMTF specification DSP0237. A MCTP protocol network device is > > created for each I2C bus that has been assigned a mctp-i2c device. > > > > +config MCTP_TRANSPORT_KCS > > + tristate "MCTP KCS transport" > > + depends on IPMI_KCS_BMC > > + help > > + Provides a driver to access MCTP devices over KCS transport, from > > + DMTF specification DSP0254. A MCTP protocol network device is > > + created for each KCS channel found in the system. > > + > > endmenu > > > > endif > > diff --git a/drivers/net/mctp/Makefile b/drivers/net/mctp/Makefile > > index 1ca3e6028f77..885339a40f22 100644 > > --- a/drivers/net/mctp/Makefile > > +++ b/drivers/net/mctp/Makefile > > @@ -1,2 +1,3 @@ > > obj-$(CONFIG_MCTP_SERIAL) += mctp-serial.o > > obj-$(CONFIG_MCTP_TRANSPORT_I2C) += mctp-i2c.o > > +obj-$(CONFIG_MCTP_TRANSPORT_KCS) += mctp-kcs.o > > diff --git a/drivers/net/mctp/mctp-kcs.c b/drivers/net/mctp/mctp-kcs.c > > new file mode 100644 > > index 000000000000..8d8b77ad709c > > --- /dev/null > > +++ b/drivers/net/mctp/mctp-kcs.c > > @@ -0,0 +1,624 @@ > > +// SPDX-License-Identifier: GPL-2.0 > > +/* > > + * Management Component Transport Protocol (MCTP) KCS transport binding. > > + * This driver is an implementation of the DMTF specificiation > > + * "DSP0254 - Management Component Transport Protocol (MCTP) KCS Transport > > + * Binding", available at: > > + * > > + * https://www.dmtf.org/sites/default/files/standards/documents/DSP0254_1.0.0.pdf > > + * > > + * This driver provides DSP0254-type MCTP-over-KCS transport using a Linux > > + * KCS client subsystem. > > + * > > + * Copyright (c) 2023 Konstantin Aladyshev <aladyshev22@gmail.com> > > + */ > > + > > +#include <linux/if_arp.h> > > +#include <linux/ipmi_kcs.h> > > +#include <linux/kcs_bmc_client.h> > > +#include <linux/mctp.h> > > +#include <linux/module.h> > > +#include <linux/mutex.h> > Check these. There aren't any mutex's in here that I noticed... > Currently there are no mutex's in the driver. Where do you think they are needed? For example there no mutex's in the 'mctp-serial.c' driver (https://github.com/torvalds/linux/blob/master/drivers/net/mctp/mctp-serial.c) > > +#include <linux/netdevice.h> > > +#include <net/mctp.h> > > +#include <net/mctpdevice.h> > > +#include <net/pkt_sched.h> > > > +// According to SMBUS spec, the polynomial is: > > +// // C(x) = X^8 + X^2 + X^1 + 1, which is 0x107, > > +// // just ignore bit8 in definition. > I'm confused by this part. Is this calling out a specification issue, or > something that the relevant specs make clear? If the second, add a reference. > If it's a specification issue that's fine, but are we sure it will get resolved > hwo you've done it here? This part of the code along with the comments was copied from the relevant edk2 driver. But it turned out that I can just use `i2c_smbus_pec` from the kernel headers. So I've dropped all of this code. > > +#define MCTP_KCS_PACKET_ERROR_CODE_POLY 0x07 > > +// > > > > + > > +static void mctp_kcs_rx_work(struct work_struct *work) > > +{ > > + struct mctp_kcs *mkcs = container_of(work, struct mctp_kcs, rx_work); > > + struct mctp_skb_cb *cb; > > + struct sk_buff *skb; > > + unsigned long flags; > > + int rc; > > + int i; > > + struct mctp_kcs_header *kcs_header; > > + int data_len; > > + int data_in_idx; > > + > > + spin_lock_irqsave(&mkcs->lock, flags); > > + for (i = 0; i < (mkcs->data_in_idx); i++) > > + dev_dbg(mkcs->client.dev->dev, "%s: data_in[%d]=0x%02x", > > + __func__, i, mkcs->data_in[i]); > > + > > + data_len = mkcs->data_in_idx - sizeof(struct mctp_kcs_header) - > > + sizeof(struct mctp_kcs_trailer); > > + if (mkcs->phase != KCS_PHASE_WRITE_DONE) { > > + dev_err(mkcs->client.dev->dev, > > + "%s: error! Wrong KCS stage at the end of data read (phase=%d)", > > + __func__, mkcs->phase); > > + mkcs->netdev->stats.rx_dropped++; > > + spin_unlock_irqrestore(&mkcs->lock, flags); > > + return; > > + } > > + > > + mkcs->phase = KCS_PHASE_WAIT_READ; > > + data_in_idx = mkcs->data_in_idx; > > + mkcs->data_in_idx = 0; > > + > > + skb = netdev_alloc_skb(mkcs->netdev, data_len); > > + if (!skb) { > > + mkcs->netdev->stats.rx_dropped++; > > + spin_unlock_irqrestore(&mkcs->lock, flags); > > I'd use a sequence of gotos and labels to deal with the different cleanup. Done > > > + return; > > + } > > + > > + kcs_header = (struct mctp_kcs_header *)mkcs->data_in; > > + rc = mctp_kcs_validate_data(mkcs, kcs_header, data_in_idx); > > + if (rc) { > > + dev_err(mkcs->client.dev->dev, > > + "%s: error! Binding validation failed", __func__); > > + dev_kfree_skb(skb); > > + spin_unlock_irqrestore(&mkcs->lock, flags); > > + return; > > + } > > + > > + skb->protocol = htons(ETH_P_MCTP); > > + skb_put_data(skb, mkcs->data_in + sizeof(struct mctp_kcs_header), > > + data_len); > > + spin_unlock_irqrestore(&mkcs->lock, flags); > > + skb_reset_network_header(skb); > > + > > + cb = __mctp_cb(skb); > > + cb->halen = 0; > > + > > + netif_rx(skb); > > + mkcs->netdev->stats.rx_packets++; > > + mkcs->netdev->stats.rx_bytes += data_len; > > +} > > + > > > +static void mctp_kcs_setup(struct net_device *ndev) > > +{ > > + ndev->type = ARPHRD_MCTP; > > + > > + /* we limit at the fixed MTU, which is also the MCTP-standard > > + * baseline MTU, so is also our minimum > > + */ > > + ndev->mtu = MCTP_KCS_MTU; > > + ndev->max_mtu = MCTP_KCS_MTU; > > + ndev->min_mtu = MCTP_KCS_MTU; > > + > > + ndev->hard_header_len = 0; > > + ndev->addr_len = 0; > > + ndev->tx_queue_len = DEFAULT_TX_QUEUE_LEN; > > + ndev->flags = IFF_NOARP; > > + ndev->netdev_ops = &mctp_kcs_netdev_ops; > > + ndev->needs_free_netdev = true; > > +} > > + > > +static void kcs_bmc_ipmi_force_abort(struct mctp_kcs *mkcs) > > +{ > > + dev_err(mkcs->client.dev->dev, > > + "Error! Force abort on KCS communication"); > > + set_state(mkcs, ERROR_STATE); > > + kcs_bmc_read_data(mkcs->client.dev); > > + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); > > + mkcs->phase = KCS_PHASE_ERROR; > > + mkcs->data_in_idx = 0; > > +} > > + > > +static void kcs_bmc_ipmi_handle_data(struct mctp_kcs *mkcs) > > +{ > > + struct kcs_bmc_device *dev; > > + u8 data; > > + > > + dev = mkcs->client.dev; > As below, I'd put this on the line defining the local variable and rename it. Done > > > + > > + switch (mkcs->phase) { > > + case KCS_PHASE_WRITE_START: > > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_START", __func__); > > + mkcs->phase = KCS_PHASE_WRITE_DATA; > > + fallthrough; > > + > > + case KCS_PHASE_WRITE_DATA: > > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_DATA", __func__); > > + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { > > + set_state(mkcs, WRITE_STATE); > > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > > + mkcs->data_in[mkcs->data_in_idx++] = > > + kcs_bmc_read_data(dev); > > + dev_dbg(dev->dev, > > + "%s: KCS_PHASE_WRITE_DATA: data_in[%d]=0x%02x", > > + __func__, mkcs->data_in_idx - 1, > > + mkcs->data_in[mkcs->data_in_idx - 1]); > > + } else { > > + kcs_bmc_ipmi_force_abort(mkcs); > > + mkcs->error = KCS_LENGTH_ERROR; > > + } > > + break; > > + > > + case KCS_PHASE_WRITE_END_CMD: > > + dev_dbg(dev->dev, "%s: KCS_PHASE_WRITE_END_CMD", __func__); > > + if (mkcs->data_in_idx < KCS_MSG_BUFSIZ) { > > + set_state(mkcs, READ_STATE); > > + mkcs->data_in[mkcs->data_in_idx++] = > > + kcs_bmc_read_data(dev); > > + dev_dbg(dev->dev, > > + "%s: KCS_PHASE_WRITE_END_CMD: data_in[%d]=0x%02x", > > + __func__, mkcs->data_in_idx - 1, > > + mkcs->data_in[mkcs->data_in_idx - 1]); > > + mkcs->phase = KCS_PHASE_WRITE_DONE; > > + schedule_work(&mkcs->rx_work); > > + } else { > > + kcs_bmc_ipmi_force_abort(mkcs); > > + mkcs->error = KCS_LENGTH_ERROR; > > + } > > + break; > > + > > + case KCS_PHASE_READ: > > + dev_dbg(dev->dev, > > + "%s: KCS_PHASE_READ, data_out_idx=%d, data_out_len=%d", > > + __func__, mkcs->data_out_idx, mkcs->data_out_len); > > + if (mkcs->data_out_idx == mkcs->data_out_len) > > + set_state(mkcs, IDLE_STATE); > > + > > + data = kcs_bmc_read_data(dev); > > + if (data != KCS_CMD_READ_BYTE) { > > + dev_dbg(dev->dev, > > + "%s: error! data is not equal to KCS_CMD_READ_BYTE", > > + __func__); > > + set_state(mkcs, ERROR_STATE); > > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > > + break; > > + } > > + > > + if (mkcs->data_out_idx == mkcs->data_out_len) { > > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > > + mkcs->netdev->stats.tx_bytes += mkcs->data_out_len; > > + mkcs->netdev->stats.tx_packets++; > > + mkcs->phase = KCS_PHASE_IDLE; > > + if (netif_queue_stopped(mkcs->netdev)) > > + netif_start_queue(mkcs->netdev); > > + break; > > + } > > + > > + dev_dbg(dev->dev, "%s: KCS_PHASE_READ: data_out[%d]=0x%02x", > > + __func__, mkcs->data_out_idx, > > + mkcs->data_out[mkcs->data_out_idx]); > > + kcs_bmc_write_data(dev, mkcs->data_out[mkcs->data_out_idx++]); > > + break; > > + > > + case KCS_PHASE_ABORT_ERROR1: > > + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR1", __func__); > > + set_state(mkcs, READ_STATE); > > + kcs_bmc_read_data(dev); > > + kcs_bmc_write_data(dev, mkcs->error); > > + mkcs->phase = KCS_PHASE_ABORT_ERROR2; > > + break; > > + > > + case KCS_PHASE_ABORT_ERROR2: > > + dev_dbg(dev->dev, "%s: KCS_PHASE_ABORT_ERROR2", __func__); > > + set_state(mkcs, IDLE_STATE); > > + kcs_bmc_read_data(dev); > > + kcs_bmc_write_data(dev, KCS_ZERO_DATA); > > + mkcs->phase = KCS_PHASE_IDLE; > > + break; > > + > > + default: > > + dev_dbg(dev->dev, "%s: unknown KCS phase", __func__); > > + kcs_bmc_ipmi_force_abort(mkcs); > > + break; > > + } > > +} > > + > > +static void kcs_bmc_ipmi_handle_cmd(struct mctp_kcs *mkcs) > > +{ > > + struct kcs_bmc_device *dev; > > + u8 cmd; > > + > > + dev = mkcs->client.dev; > Might as well save a few lines as it doesn't hurt readability. > > struct kcs_bmc_device *dev = mkcs->client.dev; > Done > > + > > + set_state(mkcs, WRITE_STATE); > > + kcs_bmc_write_data(mkcs->client.dev, KCS_ZERO_DATA); > > + > > + cmd = kcs_bmc_read_data(mkcs->client.dev); > > + switch (cmd) { > Local variable doesn't add anything really and you should use your handy > 'dev'. Maybe rename dev, as most readers will assume it's a struct device > and get confused (briefly) at the dev->dev bits in here. > > > switch (kcs_bmc_read_data(dev)) { > Done > > + case KCS_CMD_WRITE_START: > > + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_START", __func__); > > + mkcs->phase = KCS_PHASE_WRITE_START; > > + mkcs->error = KCS_NO_ERROR; > > + mkcs->data_in_idx = 0; > > + break; > > + > > + case KCS_CMD_WRITE_END: > > + dev_dbg(dev->dev, "%s: KCS_CMD_WRITE_END", __func__); > > + if (mkcs->phase != KCS_PHASE_WRITE_DATA) { > > + kcs_bmc_ipmi_force_abort(mkcs); > > + break; > > + } > > + mkcs->phase = KCS_PHASE_WRITE_END_CMD; > > + break; > > + > > + case KCS_CMD_GET_STATUS_ABORT: > > + dev_dbg(dev->dev, "%s: KCS_CMD_GET_STATUS_ABORT", __func__); > > + if (mkcs->error == KCS_NO_ERROR) > > + mkcs->error = KCS_ABORTED_BY_COMMAND; > > + > > + mkcs->phase = KCS_PHASE_ABORT_ERROR1; > > + mkcs->data_in_idx = 0; > > + break; > > + > > + default: > > + dev_dbg(dev->dev, "%s: unknown KCS command", __func__); > > + kcs_bmc_ipmi_force_abort(mkcs); > > + mkcs->error = KCS_ILLEGAL_CONTROL_CODE; > > + break; > > + } > > +} > > + > > +static inline struct mctp_kcs *client_to_mctp_kcs(struct kcs_bmc_client *client) > > +{ > > + return container_of(client, struct mctp_kcs, client); > > +} > > + > > +static irqreturn_t kcs_bmc_mctp_event(struct kcs_bmc_client *client) > > +{ > > + struct mctp_kcs *mkcs; > > + u8 status; > > + int ret; > > + > > + mkcs = client_to_mctp_kcs(client); > > + if (!mkcs) { > > + dev_err(client->dev->dev, > > + "%s: error! can't find mctp_kcs from KCS client", > > + __func__); > > + return IRQ_NONE; > > + } > > + > > + spin_lock(&mkcs->lock); > > + > > + status = kcs_bmc_read_status(client->dev); > > + if (status & KCS_STATUS_IBF) { > > + if (status & KCS_STATUS_CMD_DAT) > > + kcs_bmc_ipmi_handle_cmd(mkcs); > > + else > > + kcs_bmc_ipmi_handle_data(mkcs); > > + > > + ret = IRQ_HANDLED; > > + } else { > > + ret = IRQ_NONE; > > + } > > + > > + spin_unlock(&mkcs->lock); > > + > > + return ret; > > +} > > + > > +static const struct kcs_bmc_client_ops kcs_bmc_mctp_client_ops = { > > + .event = kcs_bmc_mctp_event, > > +}; > > + > > +static DEFINE_SPINLOCK(kcs_bmc_mctp_instances_lock); > > +static LIST_HEAD(kcs_bmc_mctp_instances); > As mentioned below, this seems to be only used to find some data again > in remove. Lots of cleaner ways to do that than a list in the driver. > I'd explore the alternatives. > This was copied from the other KCS drivers. For example please see 'kcs_bmc_cdev_ipmi.c': https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L469 > > + > > +static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) > > +{ > > + struct mctp_kcs *mkcs; > > + struct net_device *ndev; > > + char name[32]; > > + int rc; > > + > > + snprintf(name, sizeof(name), "mctpkcs%d", kcs_bmc->channel); > > + > > + ndev = alloc_netdev(sizeof(*mkcs), name, NET_NAME_ENUM, mctp_kcs_setup); > Interesting that there is an explicit devm_register_netdev() but not one for > this simple allocation case (there is one for the ethernet specific version). > Never mind, we have devm_add_action_or_reset() for that. Just create a > small wrapper for free_netdev() (which will look like devm_free_netdev() > in net/devres.c but that's local to that file) and add > > rc = devm_add_action_or_reset(&kcs_bmc->dev, > wrapper_for_free_netdev(), ndev); > if (rc) > return rc; > Did you mean something like this? ``` static void devm_free_netdev(struct device *dev, void *this) { struct net_device_devres *res = this; free_netdev(res->ndev); } ... static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) { // Instead of: //ndev = alloc_netdev //rc = register_netdev(ndev); // Use ... if (!devm_register_netdev(kcs_bmc->dev, ndev)) { dev_err_probe(kcs_bmc->dev, "alloc_netdev failed for KCS channel %d\n", kcs_bmc->channel); return -ENOMEM; } rc = devm_add_action_or_reset(&kcs_bmc->dev, devm_free_netdev(), ndev); if (rc) return rc; ... } ``` What calls do I need to perform in `kcs_bmc_mctp_remove_device` in this case? Do I still have to perform `unregister_netdev` and `free_netdev` for example? Anyway I don't see anything similar in the current mctp-i2c/mctp-serial drivers. > > + if (!ndev) { > > + dev_err(kcs_bmc->dev, > > + "alloc_netdev failed for KCS channel %d\n", > > + kcs_bmc->channel); > No idea if the kcs subsystem handles deferred probing right, but in general > anything called just in 'probe' routines can use dev_err_probe() to pretty > print errors and also register any deferred cases with the logging stuff that > lets you find out why they were deferred. > Done > > + rc = -ENOMEM; > > + goto err; > In general I find it easier to follow code that only uses a goto if there > is shared cleanup to do. > return -ENOMEM; and for this path I don't need to read further. Done > > + } > > + > > + mkcs = netdev_priv(ndev); > > + mkcs->netdev = ndev; > > + mkcs->client.dev = kcs_bmc; > > + mkcs->client.ops = &kcs_bmc_mctp_client_ops; > > + mkcs->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > > + mkcs->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > > You should not be mixing device manged cleanup and manual cleanup. Rule of thumb > is don't call any devm_ functions in a 'probe / add' type routine after you pass > the first element that requires manual cleanup. Otherwise you get horrible > race conditions or if not that, just code that is hard to check for them. > Not sure how to fix > > + if (!mkcs->data_in || !mkcs->data_out) { > Check these independently. It costs a few extra lines but give more explicit > messages and keeps ordering etc simpler. > Done > > + dev_err(kcs_bmc->dev, > > + "failed to allocate data buffers for KCS channel %d\n", > > + kcs_bmc->channel); > > + rc = -ENOMEM; > > + goto free_netdev; > > + } > > + > > + INIT_WORK(&mkcs->rx_work, mctp_kcs_rx_work); > > + > > + rc = register_netdev(ndev); > > after change above, devm_register_netdev() > > > + if (rc) > > + goto free_netdev; > > + > > + spin_lock_irq(&kcs_bmc_mctp_instances_lock); > > + list_add(&mkcs->entry, &kcs_bmc_mctp_instances); > > Add a callback and devm_add_action_or_reset() to unwind this as well. > > > + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); > > + > > + dev_info(kcs_bmc->dev, "Add MCTP client for the KCS channel %d", > > + kcs_bmc->channel); > > + return 0; > > + > > +free_netdev: > > + if (ndev) > > How do you get here with ndev not set? If there is a path, that's probably > a bug. > This check is no longer needed after the changes above. > > + free_netdev(ndev); > > + > > +err: > > + return rc; > > +} > > + > > +static int kcs_bmc_mctp_remove_device(struct kcs_bmc_device *kcs_bmc) > > +{ > > + struct mctp_kcs *mkcs = NULL, *pos; > > + > > + dev_info(kcs_bmc->dev, "Remove MCTP client for the KCS channel %d", > > + kcs_bmc->channel); > > + spin_lock_irq(&kcs_bmc_mctp_instances_lock); > > + list_for_each_entry(pos, &kcs_bmc_mctp_instances, entry) { > > + if (pos->client.dev == kcs_bmc) { > > + mkcs = pos; > > + list_del(&pos->entry); > > + break; > I don't know the kcs stuff at all but these seems 'unusual'. > Can't you stash device_set_drvdata(kcs_bmc->dev) or does it > just match the structure containing the client pointed to > by kcs_bmc_device? If so use something like > container_of(kcs_bmc->client, struct mctp_kcs, client); > Ah. You already have a function for that. Why not use that here? > > There isn't normally a reason for a driver to maintain an > additional list like this. > Once again this logic was copied from the KCS IPMI driver: https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L520 > > + } > > + } > > + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); > > + > > + if (!mkcs) > > + return -ENODEV; > > + > > + unregister_netdev(mkcs->netdev); > > + free_netdev(mkcs->netdev); > > This stuff should be opposite order of add above, or leave it to devm to clean up. Which things are exact things that are currently in the incorrect order? > > > + kcs_bmc_disable_device(mkcs->client.dev, &mkcs->client); > > This doesn't match with stuff in add - so I'd like a comment to explain > why it is here. Also needs a comment on the ordering. Perhaps this > is why you can't use devm for all the above, in which case I'd use it > nowhere in this driver. > I'm also confused on relationship between mks->client.dev and kcs_bmc > (I'm fairly sure they are the same, so just use kcs_bmc here). > I've changed the variable. Not sure about `kcs_bmc_disable_device`. I've added it since it is also present in the IPMI KCS driver. https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L533 > > > + devm_kfree(kcs_bmc->dev, mkcs->data_in); > > + devm_kfree(kcs_bmc->dev, mkcs->data_out); > > Alarm bells occur whenever an explicit devm_kfree turns up in > except in complex corner cases. Please look at how devm based > resource management works. These should not be here. > > Also, remove_device should either do things in the opposite order > to add_device, or it should have comments saying why not! > > https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L534C2-L534C2 > > + return 0; > > +} > > + > > +static const struct kcs_bmc_driver_ops kcs_bmc_mctp_driver_ops = { > > + .add_device = kcs_bmc_mctp_add_device, > > + .remove_device = kcs_bmc_mctp_remove_device, > > +}; > > + > > +static struct kcs_bmc_driver kcs_bmc_mctp_driver = { > > + .ops = &kcs_bmc_mctp_driver_ops, > > +}; > > + > > +static int __init mctp_kcs_init(void) > > +{ > > + kcs_bmc_register_driver(&kcs_bmc_mctp_driver); > > + return 0; > > +} > > + > > +static void __exit mctp_kcs_exit(void) > > +{ > > + kcs_bmc_unregister_driver(&kcs_bmc_mctp_driver); > > +} > > Hmm. So kcs is a very small subsystem hence no one has done the usual > module_kcs_driver() wrapper (see something like module_i2c_driver) > for an example. You can just use the underlying macro directly > though to get rid of most of this boilerplate. > > > module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, > kcs_bmc_uregister_driver); > Not possible. If I understand error message correctly it is from the fact that 'kcs_bmc_register_driver' returns void: ``` | drivers/net/mctp/mctp-kcs.c: In function 'kcs_bmc_mctp_driver_init': | drivers/net/mctp/mctp-kcs.c:576:36: error: void value not ignored as it ought to be | 576 | module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, kcs_bmc_unregister_driver); | include/linux/device/driver.h:265:16: note: in definition of macro 'module_driver' | 265 | return __register(&(__driver) , ##__VA_ARGS__); \ | | ^~~~~~~~~~ | include/linux/device/driver.h:266:1: error: control reaches end of non-void function [-Werror=return-type] | 266 | } \ | | ^ | drivers/net/mctp/mctp-kcs.c:576:1: note: in expansion of macro 'module_driver' | 576 | module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, kcs_bmc_unregister_driver); | | ^~~~~~~~~~~~~ | cc1: some warnings being treated as errors ``` > > + > > +module_init(mctp_kcs_init); > > +module_exit(mctp_kcs_exit); > > + > > +MODULE_LICENSE("GPL"); > > +MODULE_AUTHOR("Konstantin Aladyshev <aladyshev22@gmail.com>"); > > +MODULE_DESCRIPTION("MCTP KCS transport"); > Best regards, Konstantin Aladyshev ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-10-02 14:41 ` Konstantin Aladyshev @ 2023-10-03 16:17 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2023-10-03 16:17 UTC (permalink / raw) To: Konstantin Aladyshev Cc: minyard, joel, andrew, avifishman70, tmaimon77, tali.perry1, venture, yuenn, benjaminfair, jk, matt, davem, edumazet, kuba, pabeni, openipmi-developer, linux-kernel, linux-arm-kernel, linux-aspeed, openbmc, netdev On Mon, 2 Oct 2023 17:41:42 +0300 Konstantin Aladyshev <aladyshev22@gmail.com> wrote: > Thanks for the review! > I've corrected many things from your comments and have sent the V2 patch. > I'm not sure about the LIST thing and all the devres management. I've > written the KCS handling the same way it is done in the standard IPMI > KCS driver (https://github.com/torvalds/linux/blob/master/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c) > Not sure if we need to do any different here. > Please see detailed response below: > > > > +#include <linux/module.h> > > > +#include <linux/mutex.h> > > Check these. There aren't any mutex's in here that I noticed... > > > > Currently there are no mutex's in the driver. Where do you think they > are needed? > For example there no mutex's in the 'mctp-serial.c' driver > (https://github.com/torvalds/linux/blob/master/drivers/net/mctp/mctp-serial.c) I don't think you need a mutex. Hence don't include the header either! :) > > > > +#include <linux/netdevice.h> ... > > > + > > > +static DEFINE_SPINLOCK(kcs_bmc_mctp_instances_lock); > > > +static LIST_HEAD(kcs_bmc_mctp_instances); > > As mentioned below, this seems to be only used to find some data again > > in remove. Lots of cleaner ways to do that than a list in the driver. > > I'd explore the alternatives. > > > > This was copied from the other KCS drivers. For example please see > 'kcs_bmc_cdev_ipmi.c': > https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L469 Sure, I spotted it was copied but doesn't mean I like that code either :) > > > > + > > > +static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) > > > +{ > > > + struct mctp_kcs *mkcs; > > > + struct net_device *ndev; > > > + char name[32]; > > > + int rc; > > > + > > > + snprintf(name, sizeof(name), "mctpkcs%d", kcs_bmc->channel); > > > + > > > + ndev = alloc_netdev(sizeof(*mkcs), name, NET_NAME_ENUM, mctp_kcs_setup); > > Interesting that there is an explicit devm_register_netdev() but not one for > > this simple allocation case (there is one for the ethernet specific version). > > Never mind, we have devm_add_action_or_reset() for that. Just create a > > small wrapper for free_netdev() (which will look like devm_free_netdev() > > in net/devres.c but that's local to that file) and add > > > > rc = devm_add_action_or_reset(&kcs_bmc->dev, > > wrapper_for_free_netdev(), ndev); > > if (rc) > > return rc; > > > > > Did you mean something like this? > ``` > static void devm_free_netdev(struct device *dev, void *this) > { > struct net_device_devres *res = this; > > free_netdev(res->ndev); > } No. That would be unwind for a devm_alloc_netdev() which doesn't exist for the case where you want to override the manual version. Here would be static void kcs_bmc_mctp_free_netdev(void *priv) { free_netdev(priv); } > > > ... > > static int kcs_bmc_mctp_add_device(struct kcs_bmc_device *kcs_bmc) > { > > // Instead of: > //ndev = alloc_netdev > //rc = register_netdev(ndev); > > // Use > ... > if (!devm_register_netdev(kcs_bmc->dev, ndev)) { > dev_err_probe(kcs_bmc->dev, > "alloc_netdev failed for KCS channel %d\n", > kcs_bmc->channel); > return -ENOMEM; > } > > rc = devm_add_action_or_reset(&kcs_bmc->dev, > devm_free_netdev(), > ndev); > if (rc) > return rc; > ... > } > ``` > What calls do I need to perform in `kcs_bmc_mctp_remove_device` in this case? > Do I still have to perform `unregister_netdev` and `free_netdev` for example? Ideally none at all once everthing has moved over to device managed (devm) based handling. The purpose of devm is to automatically call all the release functions in reverse order of the setup calls (gets more complex but in this case it will simply be reverse order). That will occur on an error in probe() or after remove() callback is called. Happens without the remove() callback as well which is what we want here. > > Anyway I don't see anything similar in the current mctp-i2c/mctp-serial drivers. True - lots of examples elsewhere though :) > > > > > + if (!ndev) { > > > + dev_err(kcs_bmc->dev, > > > + "alloc_netdev failed for KCS channel %d\n", > > > + kcs_bmc->channel); > > No idea if the kcs subsystem handles deferred probing right, but in general > > anything called just in 'probe' routines can use dev_err_probe() to pretty > > print errors and also register any deferred cases with the logging stuff that > > lets you find out why they were deferred. > > > > Done > > > > + rc = -ENOMEM; > > > + goto err; > > In general I find it easier to follow code that only uses a goto if there > > is shared cleanup to do. > > return -ENOMEM; and for this path I don't need to read further. > > Done > > > > + } > > > + > > > + mkcs = netdev_priv(ndev); > > > + mkcs->netdev = ndev; > > > + mkcs->client.dev = kcs_bmc; > > > + mkcs->client.ops = &kcs_bmc_mctp_client_ops; > > > + mkcs->data_in = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > > > + mkcs->data_out = devm_kmalloc(kcs_bmc->dev, KCS_MSG_BUFSIZ, GFP_KERNEL); > > > > You should not be mixing device manged cleanup and manual cleanup. Rule of thumb > > is don't call any devm_ functions in a 'probe / add' type routine after you pass > > the first element that requires manual cleanup. Otherwise you get horrible > > race conditions or if not that, just code that is hard to check for them. > > > > Not sure how to fix Some simple rules of thumb. 1. The first call in probe() that you make that does not have automated cleanup (so non devm_ * or you haven't manually added a cleanup callback via devm_add_action_or_reset()) ends devm usage in probe. 2. In remove() and in error paths in probe() don't do anything at all to cleanup stuff that was registered with devm_ calls as they will be automatically cleaned up for you. In a simple driver it's often possible to move everything over to devm_ calls so there is no manual cleanup to do at all. If that's the case don't provide a remove() callback. However the subsystem may insist on one in which case either fix that (they should be optional) or provide an empty one. > > > + > > > +static int kcs_bmc_mctp_remove_device(struct kcs_bmc_device *kcs_bmc) > > > +{ > > > + struct mctp_kcs *mkcs = NULL, *pos; > > > + > > > + dev_info(kcs_bmc->dev, "Remove MCTP client for the KCS channel %d", > > > + kcs_bmc->channel); > > > + spin_lock_irq(&kcs_bmc_mctp_instances_lock); > > > + list_for_each_entry(pos, &kcs_bmc_mctp_instances, entry) { > > > + if (pos->client.dev == kcs_bmc) { > > > + mkcs = pos; > > > + list_del(&pos->entry); > > > + break; > > I don't know the kcs stuff at all but these seems 'unusual'. > > Can't you stash device_set_drvdata(kcs_bmc->dev) or does it > > just match the structure containing the client pointed to > > by kcs_bmc_device? If so use something like > > container_of(kcs_bmc->client, struct mctp_kcs, client); > > Ah. You already have a function for that. Why not use that here? > > > > There isn't normally a reason for a driver to maintain an > > additional list like this. > > > > Once again this logic was copied from the KCS IPMI driver: > https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L520 Understood - should be able to do better than that though ;) > > > > + } > > > + } > > > + spin_unlock_irq(&kcs_bmc_mctp_instances_lock); > > > + > > > + if (!mkcs) > > > + return -ENODEV; > > > + > > > + unregister_netdev(mkcs->netdev); > > > + free_netdev(mkcs->netdev); > > > > This stuff should be opposite order of add above, or leave it to devm to clean up. > > Which things are exact things that are currently in the incorrect order? Allocations occur in probe just before register_netdev, so they should be before free_netdev() for example. > > > > > > + kcs_bmc_disable_device(mkcs->client.dev, &mkcs->client); > > > > This doesn't match with stuff in add - so I'd like a comment to explain > > why it is here. Also needs a comment on the ordering. Perhaps this > > is why you can't use devm for all the above, in which case I'd use it > > nowhere in this driver. > > I'm also confused on relationship between mks->client.dev and kcs_bmc > > (I'm fairly sure they are the same, so just use kcs_bmc here). > > > > I've changed the variable. Not sure about `kcs_bmc_disable_device`. > I've added it since it is also present in the IPMI KCS driver. > https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L533 Understood. Would need some experimenting to figure out a path where it does something rather than it already being disabled. > > > > > > + devm_kfree(kcs_bmc->dev, mkcs->data_in); > > > + devm_kfree(kcs_bmc->dev, mkcs->data_out); > > > > Alarm bells occur whenever an explicit devm_kfree turns up in > > except in complex corner cases. Please look at how devm based > > resource management works. These should not be here. > > > > Also, remove_device should either do things in the opposite order > > to add_device, or it should have comments saying why not! > > > > > > https://github.com/torvalds/linux/blob/8a749fd1a8720d4619c91c8b6e7528c0a355c0aa/drivers/char/ipmi/kcs_bmc_cdev_ipmi.c#L534C2-L534C2 Yeah. That's bad :( Seems devm being relied on for error paths, but not remove() > > > > + return 0; > > > +} > > > + > > > +static const struct kcs_bmc_driver_ops kcs_bmc_mctp_driver_ops = { > > > + .add_device = kcs_bmc_mctp_add_device, > > > + .remove_device = kcs_bmc_mctp_remove_device, > > > +}; > > > + > > > +static struct kcs_bmc_driver kcs_bmc_mctp_driver = { > > > + .ops = &kcs_bmc_mctp_driver_ops, > > > +}; > > > + > > > +static int __init mctp_kcs_init(void) > > > +{ > > > + kcs_bmc_register_driver(&kcs_bmc_mctp_driver); > > > + return 0; > > > +} > > > + > > > +static void __exit mctp_kcs_exit(void) > > > +{ > > > + kcs_bmc_unregister_driver(&kcs_bmc_mctp_driver); > > > +} > > > > Hmm. So kcs is a very small subsystem hence no one has done the usual > > module_kcs_driver() wrapper (see something like module_i2c_driver) > > for an example. You can just use the underlying macro directly > > though to get rid of most of this boilerplate. > > > > > > module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, > > kcs_bmc_uregister_driver); > > > > Not possible. If I understand error message correctly it is from the > fact that 'kcs_bmc_register_driver' returns void: That's annoying.. Could fix it by making it return an int so it could report the failure it handles to the caller module instead of always returning success... That smells like a bug to me though I haven't checked if the module_init() return value gets used for anything much. > ``` > | drivers/net/mctp/mctp-kcs.c: In function 'kcs_bmc_mctp_driver_init': > | drivers/net/mctp/mctp-kcs.c:576:36: error: void value not ignored as > it ought to be > | 576 | module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, > kcs_bmc_unregister_driver); > | include/linux/device/driver.h:265:16: note: in definition of macro > 'module_driver' > | 265 | return __register(&(__driver) , ##__VA_ARGS__); \ > | | ^~~~~~~~~~ > | include/linux/device/driver.h:266:1: error: control reaches end of > non-void function [-Werror=return-type] > | 266 | } \ > | | ^ > | drivers/net/mctp/mctp-kcs.c:576:1: note: in expansion of macro 'module_driver' > | 576 | module_driver(kcs_bmc_mctp_driver, kcs_bmc_register_driver, > kcs_bmc_unregister_driver); > | | ^~~~~~~~~~~~~ > | cc1: some warnings being treated as errors > ``` > > > > + > > > +module_init(mctp_kcs_init); > > > +module_exit(mctp_kcs_exit); > > > + > > > +MODULE_LICENSE("GPL"); > > > +MODULE_AUTHOR("Konstantin Aladyshev <aladyshev22@gmail.com>"); > > > +MODULE_DESCRIPTION("MCTP KCS transport"); > > > > Best regards, > Konstantin Aladyshev > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-09-29 11:08 ` Jonathan Cameron 2023-10-02 14:41 ` Konstantin Aladyshev @ 2023-10-03 7:22 ` Andrew Jeffery 2023-10-03 16:21 ` Jonathan Cameron 1 sibling, 1 reply; 10+ messages in thread From: Andrew Jeffery @ 2023-10-03 7:22 UTC (permalink / raw) To: Jonathan Cameron, Konstantin Aladyshev Cc: Tomer Maimon, Corey Minyard, Patrick Venture, openbmc, linux-kernel, Tali Perry, Avi Fishman, Eric Dumazet, netdev, linux-aspeed, Joel Stanley, Jakub Kicinski, Jeremy Kerr, Matt Johnston, Paolo Abeni, openipmi-developer, David Miller, linux-arm-kernel, Benjamin Fair Hi Jonathan, On Fri, 29 Sep 2023, at 20:38, Jonathan Cameron wrote: > On Thu, 28 Sep 2023 15:30:09 +0300 > Konstantin Aladyshev <aladyshev22@gmail.com> wrote: > >> This change adds a MCTP KCS transport binding, as defined by the DMTF >> specificiation DSP0254 - "MCTP KCS Transport Binding". >> A MCTP protocol network device is created for each KCS channel found in >> the system. >> The interrupt code for the KCS state machine is based on the current >> IPMI KCS driver. >> >> Signed-off-by: Konstantin Aladyshev <aladyshev22@gmail.com> > > Drive by review as I was curious and might as well comment whilst reading. > Some comments seem to equally apply to other kcs drivers so maybe I'm > missing something... > I doubt you're missing anything. I reworked the KCS stuff a while back to make it a bit more general. Prior to Konstantin's work here the subsystem lived in its own little dark corner and might have benefitted from broader review. Some of the concerns with Konstantin's work are likely concerns with what I'd done, which he probably used as a guide. For reference the rework series is here: https://lore.kernel.org/all/20210608104757.582199-1-andrew@aj.id.au/ >> + >> +static DEFINE_SPINLOCK(kcs_bmc_mctp_instances_lock); >> +static LIST_HEAD(kcs_bmc_mctp_instances); > As mentioned below, this seems to be only used to find some data again > in remove. Lots of cleaner ways to do that than a list in the driver. > I'd explore the alternatives. Yeah, it's a little clumsy. I'll look into better ways to address the problem. > >> + if (!ndev) { >> + dev_err(kcs_bmc->dev, >> + "alloc_netdev failed for KCS channel %d\n", >> + kcs_bmc->channel); > No idea if the kcs subsystem handles deferred probing right, but in general > anything called just in 'probe' routines can use dev_err_probe() to pretty > print errors and also register any deferred cases with the logging stuff that > lets you find out why they were deferred. Let me see if there's work to do in the KCS subsystem to deal with deferred probing. I expect that there is. > > >> + if (rc) >> + goto free_netdev; >> + >> + spin_lock_irq(&kcs_bmc_mctp_instances_lock); >> + list_add(&mkcs->entry, &kcs_bmc_mctp_instances); > > Add a callback and devm_add_action_or_reset() to unwind this as well. I'll check the other KCS users as well. > > > >> + devm_kfree(kcs_bmc->dev, mkcs->data_in); >> + devm_kfree(kcs_bmc->dev, mkcs->data_out); > > Alarm bells occur whenever an explicit devm_kfree turns up in > except in complex corner cases. Please look at how devm based > resource management works. These should not be here. Ah, I think this was an oversight in how I reworked the drivers a while back. I changed the arrangement of the structures but retained the devm_* approach to resource management. Let me page the KCS stuff back in so I can clean that up. > > Also, remove_device should either do things in the opposite order > to add_device, or it should have comments saying why not! +1 > > >> + return 0; >> +} >> + >> +static const struct kcs_bmc_driver_ops kcs_bmc_mctp_driver_ops = { >> + .add_device = kcs_bmc_mctp_add_device, >> + .remove_device = kcs_bmc_mctp_remove_device, >> +}; >> + >> +static struct kcs_bmc_driver kcs_bmc_mctp_driver = { >> + .ops = &kcs_bmc_mctp_driver_ops, >> +}; >> + >> +static int __init mctp_kcs_init(void) >> +{ >> + kcs_bmc_register_driver(&kcs_bmc_mctp_driver); >> + return 0; >> +} >> + >> +static void __exit mctp_kcs_exit(void) >> +{ >> + kcs_bmc_unregister_driver(&kcs_bmc_mctp_driver); >> +} > > Hmm. So kcs is a very small subsystem hence no one has done the usual > module_kcs_driver() wrapper (see something like module_i2c_driver) > for an example. I'll probably deal with this in the course of the rest of the poking around. Thanks for the drive-by comments! Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding 2023-10-03 7:22 ` Andrew Jeffery @ 2023-10-03 16:21 ` Jonathan Cameron 0 siblings, 0 replies; 10+ messages in thread From: Jonathan Cameron @ 2023-10-03 16:21 UTC (permalink / raw) To: Andrew Jeffery Cc: Konstantin Aladyshev, Tomer Maimon, Corey Minyard, Patrick Venture, openbmc, linux-kernel, Tali Perry, Avi Fishman, Eric Dumazet, netdev, linux-aspeed, Joel Stanley, Jakub Kicinski, Jeremy Kerr, Matt Johnston, Paolo Abeni, openipmi-developer, David Miller, linux-arm-kernel, Benjamin Fair > Thanks for the drive-by comments! No problem and keep up the good work in tidying this up. Many dark and interesting corners in the kernel and not all of them get the work they deserve :) Feel free to CC me and I'll take a look at any cleanup you propose. At least KCS is small so there aren't 100s of drivers to change :) Jonathan > > Andrew ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2023-10-03 16:21 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2023-09-28 12:30 [PATCH 0/3] Add MCTP-over-KCS transport binding Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 1/3] ipmi: Move KCS headers to common include folder Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 2/3] ipmi: Create header with KCS interface defines Konstantin Aladyshev 2023-09-28 12:30 ` [PATCH 3/3] mctp: Add MCTP-over-KCS transport binding Konstantin Aladyshev 2023-09-28 22:58 ` kernel test robot 2023-09-29 11:08 ` Jonathan Cameron 2023-10-02 14:41 ` Konstantin Aladyshev 2023-10-03 16:17 ` Jonathan Cameron 2023-10-03 7:22 ` Andrew Jeffery 2023-10-03 16:21 ` Jonathan Cameron
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).