Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <997aa782-deb0-1d88-c144-8ea4537e55c2@os.amperecomputing.com>
Date:   Mon, 20 Jun 2022 14:16:36 +0700
User-Agent: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.15; rv:91.0)
 Gecko/20100101 Thunderbird/91.10.0
Subject: Re: [PATCH v8 1/3] ipmi: ssif_bmc: Add SSIF BMC driver
Content-Language: en-US
To:     Christophe JAILLET <christophe.jaillet@wanadoo.fr>
Cc:     andrew@aj.id.au, benh@kernel.crashing.org,
        brendanhiggins@google.com, devicetree@vger.kernel.org,
        joel@jms.id.au, krzysztof.kozlowski+dt@linaro.org,
        linux-arm-kernel@lists.infradead.org,
        linux-aspeed@lists.ozlabs.org, linux-i2c@vger.kernel.org,
        linux-kernel@vger.kernel.org, minyard@acm.org,
        openbmc@lists.ozlabs.org, openipmi-developer@lists.sourceforge.net,
        patches@amperecomputing.com, phong@os.amperecomputing.com,
        robh+dt@kernel.org, thang@os.amperecomputing.com, wsa@kernel.org
References: <20220615090259.1121405-1-quan@os.amperecomputing.com>
 <20220615090259.1121405-2-quan@os.amperecomputing.com>
 <7b9923c0-50f0-556a-657c-9cf0ef9af5aa@wanadoo.fr>
From:   Quan Nguyen <quan@os.amperecomputing.com>
In-Reply-To: <7b9923c0-50f0-556a-657c-9cf0ef9af5aa@wanadoo.fr>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 8bit
MIME-Version: 1.0
X-MS-Exchange-AntiSpam-MessageData-ChunkCount: 1
X-MS-Exchange-AntiSpam-MessageData-0: =?utf-8?B?R0d6d014QVlmTFB0Q21GbDI4RVhyVnMyRU4wOHBMZnNmOXJiSTI1MVBIUjA2?=
 =?utf-8?B?NEU5cW1PcERkVnV6eHJLOGhMSndtTElEeDRsT2xJaDU2ZkIwbkN0MFh5WGk1?=
 =?utf-8?B?eDVIYS94TnNtQUIwVmJhQnM4MlpDMjJyeVRNWklyQnBqOVVRSjlSWmxkQlRC?=
 =?utf-8?B?RkR4WENIdVlLbzZHYnZSb1JuVndsdytyeTVOdTNTSmNsWjZDUTc1SWxLcnk3?=
 =?utf-8?B?Yk5ZalMzUlNwc1Z6dG9IUFpHTWdrbW1QMjB2NjlwR25iN3lteU42RkNwOFRp?=
 =?utf-8?B?RXA0VWhONC9lZ1FESXNJWkR3RkxKdFlOTGs3ZS9weUl0VFd1ZUx6K0hxWEFu?=
 =?utf-8?B?MjgxUTlhMXQraXM3TGhZZ1kxd2VDcVlsMnJ1eGYxYVpvZjdNQURiNzVjdmlI?=
 =?utf-8?B?VDNPL0dRcUJPb1J3dEg5b1RNbG1sMmdUSzA0M1lJcWhkQTNwZDZ3cTBVZTM2?=
 =?utf-8?B?LzlVS0lDaVdxVFFhaTRWRXVNcEk5eWs5eUxvNUV2N3krL3luTURJdnMvanlV?=
 =?utf-8?B?eTJoT1hRbmFwNStFbGZjbXBBWnpzQmZ5VlFGRmppd2V6RFc4RzRqMFIzY3VL?=
 =?utf-8?B?eWhYeG5Ub0lyMmlhQjhmb3Y4VnN2QWQ5ZzZvSm11MXhrWmxiM0FQOUZjQlBY?=
 =?utf-8?B?bXlZRFNnc29Fdmp1NjJKWmlkNDdZZnR5TzdCK3BwRlRBcUJiY2hKUXpJWVpp?=
 =?utf-8?B?S2JGbVlqY2ZDbVhESEdpck9PR3RNSVFnY244WnVXL3ZMaUN2K1BsTng3Ykw0?=
 =?utf-8?B?VWpOVkRKTXdMRkhYQWp5V1ZBY2VoVWpGZ0liRWFKbXpEbDJ2cW00VzViRGtJ?=
 =?utf-8?B?T1hKS3M3SjV5OHcvazBKb1Y1dlB5YUZqTm9XQlh4clUxSnFjakRqM290SkZp?=
 =?utf-8?B?YXdLWWlhcG1qNUNxWHpmbUNKM2VPN25jRVpGcHlXbG82SmxtdTRmb0VydXVr?=
 =?utf-8?B?T1dWTjJlLzZpeG01aEdjL3ExbElOTmZtWkU5TGI3ZmNTTEk4VTFSMUx0bE4x?=
 =?utf-8?B?TXhEb0tObU1NWGsySjkxQVdpaVU2T3JBa25ZMkVCLzBNNXgxdkw0eE96ZmRi?=
 =?utf-8?B?UXNBdmc2WGUvVnVsNWFWcHgwOUdTeVQvZmpoV0xKR0UyVlQrN0NVVXM1Y3NL?=
 =?utf-8?B?NkVtUEhPOGtVeGdGb0VqYzNaQmRLcTBXQjNMendlRVB2RHF4Q25BdUxnU3hV?=
 =?utf-8?B?UlgzM2N0L3Axd0s5eURsSEhhcm5EbVpEZ0x3bVVrNWJIZ2xzdkxici9VNlN0?=
 =?utf-8?B?b0NDZkxwSitrSkMvZzBGbFhmazZac0VBSEttY0NrK21qSlA2dzI2RWdJVTNr?=
 =?utf-8?B?N0NqWkpub3BnblBXMXAzUVJ6OHdHR0g0YXVGSlpmSTUyZ1lsTUU4aWhqMjJC?=
 =?utf-8?B?aW9BSUZVVWxXWGw3bU4yRGpKbHgvcWdhbnpiWDNDVHBKaU5aeFpsVEE2Snk3?=
 =?utf-8?B?eS9iY1k4QnRWK212THROSnlDWkJEN1JZcG5YRHMwcXlKUVBYRlRqWEZXYjNH?=
 =?utf-8?B?a0FhbFRQanFwd25GSmltd3ZSR0JwQ1IyRitlN2lXa1l4QjVQT3IyV21NLzVi?=
 =?utf-8?B?MEkvbVVmekc5dUdvRlNPQjY3MExoMEtTUUNSWUU5RlAwMTFQa3VGY2ZNaTBL?=
 =?utf-8?B?aUEvWjgvM25WdFpZdGdza0ZDcWVjaStCaEN2R2ZkTk51ZjlBaTlZMDRYSXpZ?=
 =?utf-8?B?MkFnOVpJRkpBVVNmY01tSDNUcnFlRndrbkl2L1dLUE96QzhGRkN6Q0xtTG4z?=
 =?utf-8?B?NE1MQk9XVkRLc2hvYk03UHBnNEtvQkRaN2FqUkdVK0MvZFpBc0hqeE9pU2dx?=
 =?utf-8?B?Z0tYWXdRYW1SSDFHd1QvSE1FclNlMDlsUHVVWU53dC85WElDWnowMmhZc0NL?=
 =?utf-8?B?MGJDby9ONFVCUXEvTzdWMUVaWkVYenBZRytuakhyYXp3ODBXeVAwMGV0alZ2?=
 =?utf-8?B?V3JlRTErTmd4dEtzN0VTallpSEZsM3ZNbEZVb3B4TGl2MkFINFc3K0lTU3pD?=
 =?utf-8?B?UlFaTi9DU3pWSHhkZ1ZoaitTSktiNU1BaUkzb01xdVZtemxQbTUwY2gxYVZX?=
 =?utf-8?B?TkRWVEFGUllBUG5nYkZ1RExScm9LdU5OUWRQRFBsbkkvNGIzZk1vZFZSNmda?=
 =?utf-8?B?cXhRMkMvb0dEb055NkZOVy9pcm5aZXdtQWt6U1lCc0pDSEJlbFlqZ3RJVEw3?=
 =?utf-8?B?THpqZFFuMjFSNFlNS1JOcEhpYWFqaWVuNHEyb1U3SDFjNnNLNkpFRTlxaTh3?=
 =?utf-8?B?ZHVSOGtlWGQ1MUhkRVJINC91WXV5UUx2RE9YbEV0Qk9SRVpEQ0w1UWQ1VFVF?=
 =?utf-8?B?K0swYmNuQWNLK05qdmxPWVZzUGdJYmRUTXF1RWMvY2VRUVd2SGpUQWhBRXdX?=
 =?utf-8?Q?eq2IevJVvatIQdKE6CTATZVNegb+3wk/mZ8f3?=
X-OriginatorOrg: os.amperecomputing.com
X-MS-Exchange-CrossTenant-Network-Message-Id: d0b3ce7e-b9b9-4d83-1595-08da528cd81d
X-MS-Exchange-CrossTenant-AuthSource: SJ0PR01MB7282.prod.exchangelabs.com
X-MS-Exchange-CrossTenant-AuthAs: Internal
X-MS-Exchange-CrossTenant-OriginalArrivalTime: 20 Jun 2022 07:16:50.6966
 (UTC)
X-MS-Exchange-CrossTenant-FromEntityHeader: Hosted
X-MS-Exchange-CrossTenant-Id: 3bc2b170-fd94-476d-b0ce-4229bdc904a7
X-MS-Exchange-CrossTenant-MailboxType: HOSTED
X-MS-Exchange-CrossTenant-UserPrincipalName: UgZrz7GY/ztCVeXdwhEGpy9wXcNpWwiTOaO5vkW893Y56EOh3eF/93A8KwTvne/WCKist97SjKhrdt76qsAK/x/jMk+c2w8P9Xe/epybJL8=
X-MS-Exchange-Transport-CrossTenantHeadersStamped: CO6PR01MB7403
X-Spam-Status: No, score=-2.0 required=5.0 tests=BAYES_00,DKIM_SIGNED,
        DKIM_VALID,DKIM_VALID_AU,NICE_REPLY_A,RCVD_IN_DNSWL_NONE,
        RCVD_IN_MSPIKE_H2,SPF_HELO_PASS,SPF_PASS,T_SCC_BODY_TEXT_LINE
        autolearn=ham autolearn_force=no version=3.4.6
X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on
        lindbergh.monkeyblade.net
Precedence: bulk
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

On 17/06/2022 03:47, Christophe JAILLET wrote:
> Le 15/06/2022 à 11:02, Quan Nguyen a écrit :
>> The SMBus system interface (SSIF) IPMI BMC driver can be used to perform
>> in-band IPMI communication with their host in management (BMC) side.
>>
>> Thanks Dan for the copy_from_user() fix in the link below.
>>
>> Link: 
>> https://lore.kernel.org/linux-arm-kernel/20220310114119.13736-4-quan-shex6MNQR2J/SfDzf78azzKzEDxYleXD@public.gmane.org/ 
>>
>> Signed-off-by: Quan Nguyen 
>> <quan-shex6MNQR2J/SfDzf78azzKzEDxYleXD@public.gmane.org>
>> ---
> 
> Hi,
> 
> a few nitpick below
> 
> [...]
> 
>> diff --git a/drivers/char/ipmi/ssif_bmc.c b/drivers/char/ipmi/ssif_bmc.c
>> new file mode 100644
>> index 000000000000..0bfd4b9bbaf1
>> --- /dev/null
>> +++ b/drivers/char/ipmi/ssif_bmc.c
>> @@ -0,0 +1,880 @@
>> +// SPDX-License-Identifier: GPL-2.0-only
>> +/*
>> + * The driver for BMC side of SSIF interface
>> + *
>> + * Copyright (c) 2022, Ampere Computing LLC
>> + *
>> + */
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/miscdevice.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/platform_device.h>
>> +#include <linux/poll.h>
>> +#include <linux/sched.h>
>> +#include <linux/mutex.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/timer.h>
>> +#include <linux/jiffies.h>
>> +#include <linux/ipmi_ssif_bmc.h>
>> +
>> +#define DEVICE_NAME                             "ipmi-ssif-host"
>> +
>> +#define GET_8BIT_ADDR(addr_7bit)                (((addr_7bit) << 1) & 
>> 0xff)
>> +
>> +/* A standard SMBus Transaction is limited to 32 data bytes */
>> +#define MAX_PAYLOAD_PER_TRANSACTION             32
>> +/* Transaction includes the address, the command, the length and the 
>> PEC byte */
>> +#define MAX_TRANSACTION                         
>> (MAX_PAYLOAD_PER_TRANSACTION + 4)
>> +
>> +#define MAX_IPMI_DATA_PER_START_TRANSACTION     30
>> +#define MAX_IPMI_DATA_PER_MIDDLE_TRANSACTION    31
>> +
>> +#define SSIF_IPMI_SINGLEPART_WRITE              0x2
>> +#define SSIF_IPMI_SINGLEPART_READ               0x3
>> +#define SSIF_IPMI_MULTIPART_WRITE_START         0x6
>> +#define SSIF_IPMI_MULTIPART_WRITE_MIDDLE        0x7
>> +#define SSIF_IPMI_MULTIPART_WRITE_END           0x8
>> +#define SSIF_IPMI_MULTIPART_READ_START          0x3
>> +#define SSIF_IPMI_MULTIPART_READ_MIDDLE         0x9
>> +
>> +/*
>> + * IPMI 2.0 Spec, section 12.7 SSIF Timing,
>> + * Request-to-Response Time is T6max(250ms) - T1max(20ms) - 3ms = 227ms
>> + * Recover ssif_bmc from busy state if it takes up to 500ms
>> + */
>> +#define RESPONSE_TIMEOUT                        500 /* ms */
>> +
>> +struct ssif_part_buffer {
>> +    u8 address;
>> +    u8 smbus_cmd;
>> +    u8 length;
>> +    u8 payload[MAX_PAYLOAD_PER_TRANSACTION];
>> +    u8 pec;
>> +    u8 index;
>> +};
>> +
>> +/*
>> + * SSIF internal states:
>> + *   SSIF_READY         0x00 : Ready state
>> + *   SSIF_START         0x01 : Start smbus transaction
>> + *   SSIF_SMBUS_CMD     0x02 : Received SMBus command
>> + *   SSIF_REQ_RECVING   0x03 : Receiving request
>> + *   SSIF_RES_SENDING   0x04 : Sending response
>> + *   SSIF_BAD_SMBUS     0x05 : Bad SMbus transaction
> 
> If these states are related to the enum just below, 
> s/SSIF_BAD_SMBUS/SSIF_ABORTING/ + description update?
> 
Thank you for this catch.
Will fix in new version.

>> + */
>> +enum ssif_state {
>> +    SSIF_READY,
>> +    SSIF_START,
>> +    SSIF_SMBUS_CMD,
>> +    SSIF_REQ_RECVING,
>> +    SSIF_RES_SENDING,
>> +    SSIF_ABORTING,
>> +    SSIF_STATE_MAX
>> +};
>> +
> 
> [...]
> 
>> +static int ssif_bmc_probe(struct i2c_client *client, const struct 
>> i2c_device_id *id)
>> +{
>> +    struct ssif_bmc_ctx *ssif_bmc;
>> +    int ret;
>> +
>> +    ssif_bmc = devm_kzalloc(&client->dev, sizeof(*ssif_bmc), 
>> GFP_KERNEL);
>> +    if (!ssif_bmc)
>> +        return -ENOMEM;
>> +
>> +    spin_lock_init(&ssif_bmc->lock);
>> +
>> +    init_waitqueue_head(&ssif_bmc->wait_queue);
>> +    ssif_bmc->request_available = false;
>> +    ssif_bmc->response_in_progress = false;
>> +    ssif_bmc->busy = false;
>> +    ssif_bmc->response_timer_inited = false;
>> +
>> +    /* Register misc device interface */
>> +    ssif_bmc->miscdev.minor = MISC_DYNAMIC_MINOR;
>> +    ssif_bmc->miscdev.name = DEVICE_NAME;
>> +    ssif_bmc->miscdev.fops = &ssif_bmc_fops;
>> +    ssif_bmc->miscdev.parent = &client->dev;
>> +    ret = misc_register(&ssif_bmc->miscdev);
>> +    if (ret)
>> +        goto out;
> 
> Could be "return ret;"
> (see below)
> 
Will change to "return ret;" in next version
>> +
>> +    ssif_bmc->client = client;
>> +    ssif_bmc->client->flags |= I2C_CLIENT_SLAVE;
>> +
>> +    /* Register I2C slave */
>> +    i2c_set_clientdata(client, ssif_bmc);
>> +    ret = i2c_slave_register(client, ssif_bmc_cb);
>> +    if (ret) {
>> +        misc_deregister(&ssif_bmc->miscdev);
>> +        goto out;
>> +    }
>> +
>> +    return 0;
>> +out:
>> +    devm_kfree(&client->dev, ssif_bmc);
> 
> This looks useless to me. The whole error handling path could be 
> removed, or updated to only have the "misc_deregister()" above.
> 

Will rewrite as:

     /* Register I2C slave */
     i2c_set_clientdata(client, ssif_bmc);
     ret = i2c_slave_register(client, ssif_bmc_cb);
     if (ret)
         misc_deregister(&ssif_bmc->miscdev);

     return ret;

Thanks for the review.
- Quan