Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Sender: Corey Minyard <tcminyard@gmail.com>
Date:   Tue, 26 Oct 2021 15:48:00 -0500
From:   Corey Minyard <minyard@acm.org>
To:     Kunkun Li <likunkun@bytedance.com>
Cc:     openipmi-developer@lists.sourceforge.net,
        linux-kernel@vger.kernel.org
Subject: Re: [Openipmi-developer] [PATCH] ipmi: ssif: Add msleep in multipart
 test
Message-ID: <20211026204800.GE2744412@minyard.net>
Reply-To: minyard@acm.org
References: <20211026025834.82766-1-likunkun@bytedance.com>
 <20211026165009.GD2744412@minyard.net>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <20211026165009.GD2744412@minyard.net>
Precedence: bulk

On Tue, Oct 26, 2021 at 11:50:09AM -0500, Corey Minyard wrote:
> On Tue, Oct 26, 2021 at 10:58:34AM +0800, Kunkun Li wrote:
> > During multipart test, cmd(6,7,8) or cmd(6,7,7) will
> > be sent continuously.
> 
> This is not useful information, we don't have access to your tests, so
> this is meaningless to us.

I realized what you meant above.  The "multipart test" is what the ssif
driver does to detect multipart message support.

My suggestion would be to use the results of get device id (manufacturer
and product id) and compare it against a list of of BMCs that behave
badly when the multipart test is done against them.  Then just skip that
test for those BMCs.

I know that adding 40ms to the detection doesn't seem like much, but
every little bit matters here.  It doesn't take a lot of those to really
add up.

-corey

> 
> > 
> > The pressure test found some BMC systems cannot process
> > messages in time, resulting in read_response continues to receive
> > error messages from i2c.
> > Retry mechanism will takes 10s, and finally set not support
> > multipart transmit.
> > 
> > So, to work around this，add msleep after sending cmd 6 and
> > cmd 7 respectively. The problem did not appear again in
> > pressure test.
> 
> No, you can't slow down everyone because you have one dodgy BMC.  You
> need to detect that this is a BMC that has the problem and only do it
> for those BMCs.
> 
> -corey
> 
> > 
> > Signed-off-by: Kunkun Li <likunkun@bytedance.com>
> > ---
> >  drivers/char/ipmi/ipmi_ssif.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/char/ipmi/ipmi_ssif.c b/drivers/char/ipmi/ipmi_ssif.c
> > index 20d5af92966d..65841798fafe 100644
> > --- a/drivers/char/ipmi/ipmi_ssif.c
> > +++ b/drivers/char/ipmi/ipmi_ssif.c
> > @@ -1453,6 +1453,7 @@ static int start_multipart_test(struct i2c_client *client,
> >  	ret = i2c_smbus_write_block_data(client,
> >  					 SSIF_IPMI_MULTI_PART_REQUEST_START,
> >  					 32, msg);
> > +	msleep(SSIF_MSG_MSEC);
> >  	if (ret) {
> >  		retry_cnt--;
> >  		if (retry_cnt > 0)
> > @@ -1467,6 +1468,7 @@ static int start_multipart_test(struct i2c_client *client,
> >  	ret = i2c_smbus_write_block_data(client,
> >  					 SSIF_IPMI_MULTI_PART_REQUEST_MIDDLE,
> >  					 32, msg + 32);
> > +	msleep(SSIF_MSG_MSEC);
> >  	if (ret) {
> >  		dev_err(&client->dev, "Could not write multi-part middle, though the BMC said it could handle it.  Just limit sends to one part.\n");
> >  		return ret;
> > -- 
> > 2.11.0
> > 
> 
> 
> _______________________________________________
> Openipmi-developer mailing list
> Openipmi-developer@lists.sourceforge.net
> https://lists.sourceforge.net/lists/listinfo/openipmi-developer