Re: [PATCH v4 0/1] Add support for IPMB driver
On Thu, May 02, 2019 at 09:14:46PM +, Vadim Pasternak wrote: > [...] > > > > > > >>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? > > > I thought it wouldn't be too much of an issue to use typecast here since > > > other > > existing ipmi drivers use typecasting: bt-bmc.c, kcs_bmc_aspeed.c, > > kcs_bmc_npcm7xx.c all use (u8) typecasting. > > > But if you really think it is worth it, I could do that. > > > I just think it is not as straight forward to read this code as using a > > > simple > > typecast. Some might wonder why a GENMASK is needed in this case. > > > > > Hi Asmaa, > > I will not insist in case it's OK with maintainers. > > [...] I'm mostly against casts unless they are necessary, as they tend to clutter up the code. But I don't feel that strongly about it. I'm not sure how other maintainers feel. -corey
RE: [PATCH v4 0/1] Add support for IPMB driver
[...] > > > > >>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? > > I thought it wouldn't be too much of an issue to use typecast here since > > other > existing ipmi drivers use typecasting: bt-bmc.c, kcs_bmc_aspeed.c, > kcs_bmc_npcm7xx.c all use (u8) typecasting. > > But if you really think it is worth it, I could do that. > > I just think it is not as straight forward to read this code as using a > > simple > typecast. Some might wonder why a GENMASK is needed in this case. > > Hi Asmaa, I will not insist in case it's OK with maintainers. [...]
RE: [PATCH v4 0/1] Add support for IPMB driver
Hi Corey, Please see inline response. -Original Message- From: Corey Minyard On Behalf Of Corey Minyard Sent: Thursday, May 2, 2019 3:50 PM To: Asmaa Mnebhi Cc: Vadim Pasternak ; w...@the-dreams.de; Michael Shych ; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: Re: [PATCH v4 0/1] Add support for IPMB driver On Thu, May 02, 2019 at 06:06:12PM +, Asmaa Mnebhi wrote: > Hi Vadim, Hi Corey, > > Please find inline comments answering your questions. > > -Original Message- > From: Vadim Pasternak > Sent: Tuesday, April 30, 2019 5:24 PM > To: Asmaa Mnebhi ; miny...@acm.org; > w...@the-dreams.de; Michael Shych > Cc: Asmaa Mnebhi ; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org > Subject: RE: [PATCH v4 0/1] Add support for IPMB driver > > > > > -Original Message- > > From: Asmaa Mnebhi > > Sent: Tuesday, April 30, 2019 8:59 PM > > To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak > > ; Michael Shych > > Cc: Asmaa Mnebhi ; 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? > @Vadim Pasternak: I would. > @Corey: The ipmb-dev-int driver is not responsible for sending requests via > I2C (unlike for example the ipmi_ssif driver). It is only responsible for > receiving those requests and passing them to a user space program. Once a > response is received from user space, it will forward that response back to > the requester So in my testing for example, the source requester is the BMC, > so I issue ipmitool commands from the BMC itself. > > The driver that I have on my BMC (which I primarily designed for testing > ipmb-dev-int) works hand in hand with the ipmi_msghandler and ipmi_devint to > create the /dev/ipmi0 device file to enable the use of ipmitool program on > the BMC. Once an ipmitool command is issued on the BMC, the request message > is sent to the Satellite MC. Once the BMC driver gets the response back from > the Satellite MC, it will pass it to the ipmitool program which will display > the output to the user. > > Please note that this ipmb-dev-int driver does not need ipmi_msghandler nor > does it need ipmi_devintf to be loaded. Ah, I misunderstood. I thought you were talking about the IPMB driver on the BMC. So what you have is something like: Host BMC Sat MC --- -- ipmb-dev-int <> ipmb-dev-int Linux IPMI drv <---> ipmi-host-int I assume you can use the same ipmb-dev-int on the BMC and satellite MCs (assuming Linux runs on both). You have another driver that runs on the BMC that talks to the host system through some sort of semi-custom interface, and then another driver that's on the Linux host that sits under ipmi_msghandler and provides the host access to that interface. That's a separate issue, as you said. -corey Yes exactly! What you have described above is an IPMB bridged request from a host to the Satellite MC, the BMC playing the role of the controller. And yes, in that case we can definitely use the ipmb-dev-int on the BMC as well to receive messages from the host and send the response back. In my testing, I took a short cut and wrote a customized driver that I load on the BMC, which allows me to issue ipmitool commands on the BMC itself as opposed to from a host. Thanks, Asmaa > > > 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. >
Re: [PATCH v4 0/1] Add support for IPMB driver
On Thu, May 02, 2019 at 06:06:12PM +, Asmaa Mnebhi wrote: > Hi Vadim, Hi Corey, > > Please find inline comments answering your questions. > > -Original Message- > From: Vadim Pasternak > Sent: Tuesday, April 30, 2019 5:24 PM > To: Asmaa Mnebhi ; miny...@acm.org; w...@the-dreams.de; > Michael Shych > Cc: Asmaa Mnebhi ; linux-kernel@vger.kernel.org; > linux-...@vger.kernel.org > Subject: RE: [PATCH v4 0/1] Add support for IPMB driver > > > > > -Original Message- > > From: Asmaa Mnebhi > > Sent: Tuesday, April 30, 2019 8:59 PM > > To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak > > ; Michael Shych > > Cc: Asmaa Mnebhi ; 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? > @Vadim Pasternak: I would. > @Corey: The ipmb-dev-int driver is not responsible for sending requests via > I2C (unlike for example the ipmi_ssif driver). It is only responsible for > receiving those requests and passing them to a user space program. Once a > response is received from user space, it will forward that response back to > the requester So in my testing for example, the source requester is the BMC, > so I issue ipmitool commands from the BMC itself. > > The driver that I have on my BMC (which I primarily designed for testing > ipmb-dev-int) works hand in hand with the ipmi_msghandler and ipmi_devint to > create the /dev/ipmi0 device file to enable the use of ipmitool program on > the BMC. Once an ipmitool command is issued on the BMC, the request message > is sent to the Satellite MC. Once the BMC driver gets the response back from > the Satellite MC, it will pass it to the ipmitool program which will display > the output to the user. > > Please note that this ipmb-dev-int driver does not need ipmi_msghandler nor > does it need ipmi_devintf to be loaded. Ah, I misunderstood. I thought you were talking about the IPMB driver on the BMC. So what you have is something like: Host BMC Sat MC --- -- ipmb-dev-int <> ipmb-dev-int Linux IPMI drv <---> ipmi-host-int I assume you can use the same ipmb-dev-int on the BMC and satellite MCs (assuming Linux runs on both). You have another driver that runs on the BMC that talks to the host system through some sort of semi-custom interface, and then another driver that's on the Linux host that sits under ipmi_msghandler and provides the host access to that interface. That's a separate issue, as you said. -corey > > > 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 lik
RE: [PATCH v4 0/1] Add support for IPMB driver
Hi Vadim, Hi Corey, Please find inline comments answering your questions. -Original Message- From: Vadim Pasternak Sent: Tuesday, April 30, 2019 5:24 PM To: Asmaa Mnebhi ; miny...@acm.org; w...@the-dreams.de; Michael Shych Cc: Asmaa Mnebhi ; linux-kernel@vger.kernel.org; linux-...@vger.kernel.org Subject: RE: [PATCH v4 0/1] Add support for IPMB driver > -Original Message- > From: Asmaa Mnebhi > Sent: Tuesday, April 30, 2019 8:59 PM > To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak > ; Michael Shych > Cc: Asmaa Mnebhi ; 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? @Vadim Pasternak: I would. @Corey: The ipmb-dev-int driver is not responsible for sending requests via I2C (unlike for example the ipmi_ssif driver). It is only responsible for receiving those requests and passing them to a user space program. Once a response is received from user space, it will forward that response back to the requester So in my testing for example, the source requester is the BMC, so I issue ipmitool commands from the BMC itself. The driver that I have on my BMC (which I primarily designed for testing ipmb-dev-int) works hand in hand with the ipmi_msghandler and ipmi_devint to create the /dev/ipmi0 device file to enable the use of ipmitool program on the BMC. Once an ipmitool command is issued on the BMC, the request message is sent to the Satellite MC. Once the BMC driver gets the response back from the Satellite MC, it will pass it to the ipmitool program which will display the output to the user. Please note that this ipmb-dev-int driver does not need ipmi_msghandler nor does it need ipmi_devintf to be loaded. > 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? I thought it wouldn't be too much of an issue to use typecast here since other existing ipmi drivers use typecasting: bt-bmc.c, kcs_bmc_aspeed.c, kcs_bmc_npcm7xx.c all use (u8) typecasting. But if you really think it is worth it, I could do that. I just think it is not as straight forward to read this code as using a simple typecast. Some might wonder why a GENMASK is needed in this case. > > "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. I see, I understand now. That sounds good. I added support to instantiate several ipmb devices for the purpose of having multiple BMC mast
Re: [PATCH v4 0/1] Add support for IPMB driver
On Tue, Apr 30, 2019 at 09:24:29PM +, Vadim Pasternak wrote: > > > > -Original Message- > > From: Asmaa Mnebhi > > Sent: Tuesday, April 30, 2019 8:59 PM > > To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak > > ; Michael Shych > > Cc: Asmaa Mnebhi ; 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 is a little confusing to me. Why wouldn't you use the same driver on the BMC? IIRC, the IPMB protocol is symmetric at this level. -corey > > > 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. I second this. Especially if it's on a BMC, you can expect to have multiple IPMBs. -corey > > > > > 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 >
RE: [PATCH v4 0/1] Add support for IPMB driver
> -Original Message- > From: Asmaa Mnebhi > Sent: Tuesday, April 30, 2019 8:59 PM > To: miny...@acm.org; w...@the-dreams.de; Vadim Pasternak > ; Michael Shych > Cc: Asmaa Mnebhi ; 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
