From: Roderick Colenbrander <thunderbird2k@gmail.com>
To: Max Staudt <max@enpas.org>
Cc: Roderick Colenbrander <roderick.colenbrander@sony.com>,
Jiri Kosina <jikos@kernel.org>,
Benjamin Tissoires <benjamin.tissoires@redhat.com>,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v1] HID: playstation: DS4: Fix calibration workaround for clone devices
Date: Mon, 15 Apr 2024 12:47:29 -0700 [thread overview]
Message-ID: <CAEc3jaBbOfouWJtJWjGvYkqLtd9WnV=8JC94nJuhtPvpp-39AQ@mail.gmail.com> (raw)
In-Reply-To: <20240414161223.117654-1-max@enpas.org>
Hi Max,
Thanks for your patch. For readability, I would rather not move a lot
of the ABS_X/ABS_RX related initialization code around. It is not my
preference, but I think keeping the lines were they are, but doing
this within the 'fail safe loops' is nicer:
ds4->gyro_calib_data[i].abs_code = ABS_RX + i;
Thanks,
Roderick
On Sun, Apr 14, 2024 at 9:12 AM Max Staudt <max@enpas.org> wrote:
>
> The logic in dualshock4_get_calibration_data() used uninitialised data
> in case of a failed kzalloc() for the transfer buffer.
>
> The solution is to group all business logic and all sanity checks
> together, and jump only to the latter in case of an error.
> While we're at it, factor out the axes' labelling, since it must happen
> either way for input_report_abs() to succeed later on.
>
> Thanks to Dan Carpenter for the Smatch static checker warning.
>
> Fixes: a48a7cd85f55 ("HID: playstation: DS4: Don't fail on calibration data request")
> Signed-off-by: Max Staudt <max@enpas.org>
> ---
> drivers/hid/hid-playstation.c | 63 ++++++++++++++++++-----------------
> 1 file changed, 33 insertions(+), 30 deletions(-)
>
> diff --git a/drivers/hid/hid-playstation.c b/drivers/hid/hid-playstation.c
> index edc46fc02e9a..998e79be45ac 100644
> --- a/drivers/hid/hid-playstation.c
> +++ b/drivers/hid/hid-playstation.c
> @@ -1787,7 +1787,7 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_SIZE, GFP_KERNEL);
> if (!buf) {
> ret = -ENOMEM;
> - goto no_buffer_tail_check;
> + goto transfer_failed;
> }
>
> /* We should normally receive the feature report data we asked
> @@ -1807,6 +1807,7 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
>
> hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
> ret = -EILSEQ;
> + goto transfer_failed;
> } else {
> break;
> }
> @@ -1815,17 +1816,19 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> buf = kzalloc(DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, GFP_KERNEL);
> if (!buf) {
> ret = -ENOMEM;
> - goto no_buffer_tail_check;
> + goto transfer_failed;
> }
>
> ret = ps_get_report(hdev, DS4_FEATURE_REPORT_CALIBRATION_BT, buf,
> DS4_FEATURE_REPORT_CALIBRATION_BT_SIZE, true);
>
> - if (ret)
> + if (ret) {
> hid_warn(hdev, "Failed to retrieve DualShock4 calibration info: %d\n", ret);
> + goto transfer_failed;
> + }
> }
>
> - /* Parse buffer. If the transfer failed, this safely copies zeroes. */
> + /* Transfer succeeded - parse the calibration data received. */
> gyro_pitch_bias = get_unaligned_le16(&buf[1]);
> gyro_yaw_bias = get_unaligned_le16(&buf[3]);
> gyro_roll_bias = get_unaligned_le16(&buf[5]);
> @@ -1854,71 +1857,71 @@ static int dualshock4_get_calibration_data(struct dualshock4 *ds4)
> acc_z_plus = get_unaligned_le16(&buf[31]);
> acc_z_minus = get_unaligned_le16(&buf[33]);
>
> + /* Done parsing the buffer, so let's free it. */
> + kfree(buf);
> +
> /*
> * Set gyroscope calibration and normalization parameters.
> * Data values will be normalized to 1/DS4_GYRO_RES_PER_DEG_S degree/s.
> */
> speed_2x = (gyro_speed_plus + gyro_speed_minus);
> - ds4->gyro_calib_data[0].abs_code = ABS_RX;
> ds4->gyro_calib_data[0].bias = 0;
> ds4->gyro_calib_data[0].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[0].sens_denom = abs(gyro_pitch_plus - gyro_pitch_bias) +
> abs(gyro_pitch_minus - gyro_pitch_bias);
>
> - ds4->gyro_calib_data[1].abs_code = ABS_RY;
> ds4->gyro_calib_data[1].bias = 0;
> ds4->gyro_calib_data[1].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[1].sens_denom = abs(gyro_yaw_plus - gyro_yaw_bias) +
> abs(gyro_yaw_minus - gyro_yaw_bias);
>
> - ds4->gyro_calib_data[2].abs_code = ABS_RZ;
> ds4->gyro_calib_data[2].bias = 0;
> ds4->gyro_calib_data[2].sens_numer = speed_2x*DS4_GYRO_RES_PER_DEG_S;
> ds4->gyro_calib_data[2].sens_denom = abs(gyro_roll_plus - gyro_roll_bias) +
> abs(gyro_roll_minus - gyro_roll_bias);
>
> - /* Done parsing the buffer, so let's free it. */
> - kfree(buf);
> -
> -no_buffer_tail_check:
> -
> - /*
> - * Sanity check gyro calibration data. This is needed to prevent crashes
> - * during report handling of virtual, clone or broken devices not implementing
> - * calibration data properly.
> - */
> - for (i = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> - if (ds4->gyro_calib_data[i].sens_denom == 0) {
> - hid_warn(hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
> - ds4->gyro_calib_data[i].abs_code);
> - ds4->gyro_calib_data[i].bias = 0;
> - ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
> - ds4->gyro_calib_data[i].sens_denom = S16_MAX;
> - }
> - }
> -
> /*
> * Set accelerometer calibration and normalization parameters.
> * Data values will be normalized to 1/DS4_ACC_RES_PER_G g.
> */
> range_2g = acc_x_plus - acc_x_minus;
> - ds4->accel_calib_data[0].abs_code = ABS_X;
> ds4->accel_calib_data[0].bias = acc_x_plus - range_2g / 2;
> ds4->accel_calib_data[0].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[0].sens_denom = range_2g;
>
> range_2g = acc_y_plus - acc_y_minus;
> - ds4->accel_calib_data[1].abs_code = ABS_Y;
> ds4->accel_calib_data[1].bias = acc_y_plus - range_2g / 2;
> ds4->accel_calib_data[1].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[1].sens_denom = range_2g;
>
> range_2g = acc_z_plus - acc_z_minus;
> - ds4->accel_calib_data[2].abs_code = ABS_Z;
> ds4->accel_calib_data[2].bias = acc_z_plus - range_2g / 2;
> ds4->accel_calib_data[2].sens_numer = 2*DS4_ACC_RES_PER_G;
> ds4->accel_calib_data[2].sens_denom = range_2g;
>
> +transfer_failed:
> + ds4->gyro_calib_data[0].abs_code = ABS_RX;
> + ds4->gyro_calib_data[1].abs_code = ABS_RY;
> + ds4->gyro_calib_data[2].abs_code = ABS_RZ;
> + ds4->accel_calib_data[0].abs_code = ABS_X;
> + ds4->accel_calib_data[1].abs_code = ABS_Y;
> + ds4->accel_calib_data[2].abs_code = ABS_Z;
> +
> + /*
> + * Sanity check gyro calibration data. This is needed to prevent crashes
> + * during report handling of virtual, clone or broken devices not implementing
> + * calibration data properly.
> + */
> + for (i = 0; i < ARRAY_SIZE(ds4->gyro_calib_data); i++) {
> + if (ds4->gyro_calib_data[i].sens_denom == 0) {
> + hid_warn(hdev, "Invalid gyro calibration data for axis (%d), disabling calibration.",
> + ds4->gyro_calib_data[i].abs_code);
> + ds4->gyro_calib_data[i].bias = 0;
> + ds4->gyro_calib_data[i].sens_numer = DS4_GYRO_RANGE;
> + ds4->gyro_calib_data[i].sens_denom = S16_MAX;
> + }
> + }
> +
> /*
> * Sanity check accelerometer calibration data. This is needed to prevent crashes
> * during report handling of virtual, clone or broken devices not implementing calibration
> --
> 2.39.2
>
>
prev parent reply other threads:[~2024-04-15 19:47 UTC|newest]
Thread overview: 2+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-14 16:12 [PATCH v1] HID: playstation: DS4: Fix calibration workaround for clone devices Max Staudt
2024-04-15 19:47 ` Roderick Colenbrander [this message]
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAEc3jaBbOfouWJtJWjGvYkqLtd9WnV=8JC94nJuhtPvpp-39AQ@mail.gmail.com' \
--to=thunderbird2k@gmail.com \
--cc=benjamin.tissoires@redhat.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=max@enpas.org \
--cc=roderick.colenbrander@sony.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).