On Mon, Apr 4, 2022 at 12:34 PM Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
On 04/04/2022 11:16, Andy Shevchenko wrote:
On Sun, Apr 3, 2022 at 9:38 PM Krzysztof Kozlowski krzysztof.kozlowski@linaro.org wrote:
...
+int driver_set_override(struct device *dev, const char **override,
const char *s, size_t len)+{
const char *new, *old;char *cp;
if (!override || !s)return -EINVAL;Still not sure if we should distinguish (s == NULL && len == 0) from (s != NULL && len == 0). Supplying the latter seems confusing (yes, I see that in the old code). Perhaps !s test, in case you want to leave it, should be also commented.
The old semantics were focused on sysfs usage, so clearing is by passing an empty string. In the case of sysfs empty string is actually "\n". I intend to keep the semantics also for the in-kernel usage and in such case empty string can be also "".
If I understand your comment correctly, you propose to change it to NULL for in-kernel usage, but that would change the semantics.
Yes. It's also possible to have a wrapper for sysfs use.
Another approach is to split above to two checks and move !s after !len.
I don't follow why... The !override and !s are invalid uses. !len is a valid user for internal callers, just like "\n" is for sysfs.
I understand but always supplying s maybe an overhead for in-kernel usages.
In any case, it's not critical right now, just a remark that it can be modified.
/** The stored value will be used in sysfs show callback (sysfs_emit()),* which has a length limit of PAGE_SIZE and adds a trailing newline.* Thus we can store one character less to avoid truncation during sysfs* show.*/if (len >= (PAGE_SIZE - 1))return -EINVAL;Perhaps explain the case in the comment here?
You mean the case we discuss here (to clear override with "")? Sure.
Yep. Before the below check.
if (!len) {device_lock(dev);old = *override;*override = NULL;
device_unlock(dev);goto out_free;You may deduplicate this one, by
goto out_unlock_free;But I understand your intention to keep lock-unlock in one place, so perhaps dropping that label would be even better in this case and keeping it
Yes, exactly.
kfree(old); return 0;here instead of goto.
Slightly more code, but indeed maybe easier to follow. I'll do like this.