On 2/12/24 06:12, Aleksandr Mezin wrote:
Instead of pre-allocating a buffer and synchronizing the access to it,
simply allocate memory when needed.
Fewer synchronization primitives, fewer lines of code.
Trading that for runtime overhead and an additional failure point ?
I don't think that is super-valuable, and it doesn't really save
anything in the execution path except making it more expensive.
You could save as many lines of code by allocating the buffer
as part of struct kraken3_data, i.e., with
u8 buffer[MAX_REPORT_LENGTH];
instead of
u8 *buffer;
I really dislike temporary memory allocations like that, due to the
added runtime overhead, added failure risk, and the potential for
memory fragmentation, so unless you provide a much better reason
for this change I am not going to accept it.
Guenter
Signed-off-by: Aleksandr Mezin <mezin.alexander@xxxxxxxxx>
---
drivers/hwmon/nzxt-kraken3.c | 19 ++++++-------------
1 file changed, 6 insertions(+), 13 deletions(-)
diff --git a/drivers/hwmon/nzxt-kraken3.c b/drivers/hwmon/nzxt-kraken3.c
index 5806a3f32bcb..1b2aacf9f44c 100644
--- a/drivers/hwmon/nzxt-kraken3.c
+++ b/drivers/hwmon/nzxt-kraken3.c
@@ -97,7 +97,6 @@ struct kraken3_data {
struct hid_device *hdev;
struct device *hwmon_dev;
struct dentry *debugfs;
- struct mutex buffer_lock; /* For locking access to buffer */
struct mutex z53_status_request_lock;
struct completion fw_version_processed;
/*
@@ -109,7 +108,6 @@ struct kraken3_data {
/* For locking the above completion */
spinlock_t status_completion_lock;
- u8 *buffer;
struct kraken3_channel_info channel_info[2]; /* Pump and fan */
bool is_device_faulty;
@@ -186,13 +184,15 @@ static umode_t kraken3_is_visible(const void *data, enum hwmon_sensor_types type
static int kraken3_write_expanded(struct kraken3_data *priv, const u8 *cmd, int cmd_length)
{
int ret;
+ u8 *buffer = kmalloc(MAX_REPORT_LENGTH, GFP_KERNEL);
- mutex_lock(&priv->buffer_lock);
+ if (buffer == NULL)
+ return -ENOMEM;
- memcpy_and_pad(priv->buffer, MAX_REPORT_LENGTH, cmd, cmd_length, 0x00);
- ret = hid_hw_output_report(priv->hdev, priv->buffer, MAX_REPORT_LENGTH);
+ memcpy_and_pad(buffer, MAX_REPORT_LENGTH, cmd, cmd_length, 0x00);
+ ret = hid_hw_output_report(priv->hdev, buffer, MAX_REPORT_LENGTH);
- mutex_unlock(&priv->buffer_lock);
+ kfree(buffer);
return ret;
}
@@ -913,13 +913,6 @@ static int kraken3_probe(struct hid_device *hdev, const struct hid_device_id *id
break;
}
- priv->buffer = devm_kzalloc(&hdev->dev, MAX_REPORT_LENGTH, GFP_KERNEL);
- if (!priv->buffer) {
- ret = -ENOMEM;
- goto fail_and_close;
- }
-
- mutex_init(&priv->buffer_lock);
mutex_init(&priv->z53_status_request_lock);
init_completion(&priv->fw_version_processed);
init_completion(&priv->status_report_processed);
