linux-kernel - Re: [PATCH v2 2/5] input: misc: Add IBM Operation Panel driver

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:   Tue, 8 Sep 2020 22:40:51 +0200
From:   wsa@...nel.org
To:     Eddie James <eajames@...ux.ibm.com>
Cc:     linux-input@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-aspeed@...ts.ozlabs.org,
        linux-i2c@...r.kernel.org, joel@....id.au, andrew@...id.au,
        benh@...nel.crashing.org, brendanhiggins@...gle.com,
        dmitry.torokhov@...il.com, robh+dt@...nel.org,
        rentao.bupt@...il.com, ryan_chen@...eedtech.com
Subject: Re: [PATCH v2 2/5] input: misc: Add IBM Operation Panel driver

Hi Eddie,

> +	switch (event) {
> +	case I2C_SLAVE_STOP:
> +		command_size = panel->idx;
> +		fallthrough;
> +	case I2C_SLAVE_WRITE_REQUESTED:
> +		panel->idx = 0;
> +		break;
> +	case I2C_SLAVE_WRITE_RECEIVED:
> +		if (panel->idx < sizeof(panel->command))
> +			panel->command[panel->idx++] = *val;
> +		else
> +			/*
> +			 * The command is too long and therefore invalid, so set the index
> +			 * to it's largest possible value. When a STOP is finally received,
> +			 * the command will be rejected upon processing.
> +			 */
> +			panel->idx = U8_MAX;
> +		break;
> +	default:
> +		break;
> +	}

Sorry, I missed this in my last review. READ states are mandatory, so
you will need something like this:

+	case I2C_SLAVE_READ_REQUESTED:
+	case I2C_SLAVE_READ_PROCESSED:
+		*val = 0xff;
+		break;


> +	if (command_size)
> +		ibm_panel_process_command(panel, command_size);

I wondered if you could check for the correct command_size here, so no
need to call into the function when the size doesn't match?

> +       rc = i2c_slave_register(client, ibm_panel_i2c_slave_cb);
> +       if (rc) {
> +               input_unregister_device(panel->input);
> +               dev_err(&client->dev,
> +                       "Failed to register I2C slave device: %d\n", rc);

This dev_err can go. The core will print messages if something goes
wrong.

The rest looks good from an I2C point of view.

I'd think this all should go via the input tree?

Kind regards,

   Wolfram


Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)