linux-kernel - Re: [PATCH v6 1/4] ipmi: ssif

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 17 Mar 2022 16:00:08 +0700
From:   Quan Nguyen <quan@...amperecomputing.com>
To:     Jae Hyun Yoo <quic_jaehyoo@...cinc.com>,
        Corey Minyard <minyard@....org>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski@...onical.com>,
        Joel Stanley <joel@....id.au>,
        Andrew Jeffery <andrew@...id.au>,
        Brendan Higgins <brendanhiggins@...gle.com>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Wolfram Sang <wsa@...nel.org>,
        openipmi-developer@...ts.sourceforge.net,
        devicetree@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
        linux-aspeed@...ts.ozlabs.org, linux-kernel@...r.kernel.org,
        linux-i2c@...r.kernel.org, openbmc@...ts.ozlabs.org
Cc:     Open Source Submission <patches@...erecomputing.com>,
        "Thang Q . Nguyen" <thang@...amperecomputing.com>,
        Phong Vo <phong@...amperecomputing.com>
Subject: Re: [PATCH v6 1/4] ipmi: ssif_bmc: Add SSIF BMC driver



On 15/03/2022 01:15, Jae Hyun Yoo wrote:
> Hi Quan,
> 
> [...]
> 
>> +static void on_read_requested_event(struct ssif_bmc_ctx *ssif_bmc, u8 
>> *val)
>> +{
>> +    if (ssif_bmc->state == SSIF_READY ||
>> +        ssif_bmc->state == SSIF_START ||
>> +        ssif_bmc->state == SSIF_REQ_RECVING ||
>> +        ssif_bmc->state == SSIF_RES_SENDING) {
>> +        ssif_bmc->state = SSIF_BAD_SMBUS;
>> +        dev_warn(&ssif_bmc->client->dev,
>> +             "Warn: %s unexpected READ REQUESTED in state=%s\n",
>> +             __func__, state_to_string(ssif_bmc->state));
> 
> It will print out that the case happens in SSIF_BAD_SMBUS state always
> because ssif_bmc->state is already assigned as SSIF_BAD_SMBUS above.
> Move the assignment to after the dev_warn printing.
> 
> [...]
> 

Thanks for pointing this out. Will fix this in next version.

>> +static const struct of_device_id ssif_bmc_match[] = {
>> +    { .compatible = "ampere,ssif-bmc" },
> 
> Does this driver have any Ampere specific handling? If not, it could be
> "ssif-bmc" as a generic SSIF driver.
> 
Will change as you suggested in next version.

>> +    { },
>> +};
> 
> Add 'MODULE_DEVICE_TABLE(of, ssif_bmc_match);'
> 
Will do in next version
>> +
>> +static const struct i2c_device_id ssif_bmc_id[] = {
>> +    { DEVICE_NAME, 0 },
>> +    { },
>> +};
>> +
> 
> Remove this blank line.
> 
Will update in next version

>> +MODULE_DEVICE_TABLE(i2c, ssif_bmc_id);
>> +
>> +static struct i2c_driver ssif_bmc_driver = {
>> +    .driver         = {
>> +        .name           = DEVICE_NAME,
>> +        .of_match_table = ssif_bmc_match,
>> +    },
>> +    .probe          = ssif_bmc_probe,
>> +    .remove         = ssif_bmc_remove,
>> +    .id_table       = ssif_bmc_id,
>> +};
>> +
> 
> Remove this blank line too.
> 
> -Jae
>

Thanks Jae for the review, will update in next version.
- Quan

>> +module_i2c_driver(ssif_bmc_driver);
>> +
>> +MODULE_AUTHOR("Quan Nguyen <quan@...amperecomputing.com>");
>> +MODULE_AUTHOR("Chuong Tran <chuong@...amperecomputing.com>");
>> +MODULE_DESCRIPTION("Linux device driver of the BMC IPMI SSIF 
>> interface.");
>> +MODULE_LICENSE("GPL");
> 
> [...]