> -----Original Message-----
> From: Asmaa Mnebhi <as...@mellanox.com>
> Sent: Tuesday, April 30, 2019 8:59 PM
> To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak
> <vad...@mellanox.com>; Michael Shych <michae...@mellanox.com>
> Cc: Asmaa Mnebhi <as...@mellanox.com>; linux-kernel@vger.kernel.org;
> linux-...@vger.kernel.org
> Subject: [PATCH v4 0/1] Add support for IPMB driver
>
> Thank you for your feedback Vadim. I have addressed your comments.
Hi Asmaa,
Thank you for your comments and added doc.
>
> 1) You are correct. This driver is not specific to Mellanox so I have removed
> the
> Mellanox attribute.
>
> 2) I have added a documentation file called IPMB.txt which explains how this
> module works and how it should be instantiated. It is very similar to the
> existing
> linux i2c-slave-eeprom module.
>
> The HW for my testing works as follows:
> A BMC is connected to a Satellite MC via I2C (I2C is equivalent to IPMB). The
> BMC initiates the IPMB requests and sends them via I2C.
> Obviously the BMC needs its own driver to do this which I haven't included in
> this
> code. We have no intent of upstreaming that at the moment.
I believe you are going to do it at some point, right?
> This ipmb-dev-int driver is to be loaded and instantiated on the Satellite MC
> to
> be able to receive IPMB requests. These IPMB request messages will be picked
> up by a user space program such (in my case it is OpenIPMI) to handle the
> request and generate a response.
> The response will be then passed from the user program back to kernel space.
> Then this driver would send that response back to the BMC.
>
> 3) You asked the following:
>
> "Is it expected to be zero in vaid case?"
> The 8 least significant bits of the sum is always expected to be 0 in the case
> where the checksum is valid. I have added a comment for clarifications.
>
> "why do you need this cast?"
> buf[++ipmb_dev_p->msg_idx]=(u8)(client->addr<<1)
> This is because client->addr is of type unsigned short which is
> 2 bytes so it is safer to typecast it to u8 (u8* buf)
Better, if you can avoid cast.
Would compiler warn if you use for example
rol16(client->addr, 1) & GENMASK(7, 0);
or something like it?
>
> "It could be only single ipmb-dev within the system? Couldn't it be few, like
> master/slave for example?"
> My understanding of your question is that: what if we have multiple instances
> of
> ipmb-dev-int, that we register it under different addresses?
> This driver only works as a slave so it will only be instantiated once on the
> Satellite MC under one slave address.
I mentioned some config like:
BMC1 (master) -- busA --|
Satellite
BMC2 (standby) -- busB --|
Since this is not Mellanox specific driver, maybe it would be good to support
multiple instances of it.
>
> Asmaa Mnebhi (1):
> Add support for IPMB driver
>
> Documentation/IPMB.txt | 53 ++++++
> drivers/char/ipmi/Kconfig | 8 +
> drivers/char/ipmi/Makefile | 1 +
> drivers/char/ipmi/ipmb_dev_int.c | 381
> +++++++++++++++++++++++++++++++++++++++
> 4 files changed, 443 insertions(+)
> create mode 100644 Documentation/IPMB.txt create mode 100644
> drivers/char/ipmi/ipmb_dev_int.c
>
> --
> 2.1.2
