Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
On 04.05.2020 13:53, Daniel Vetter wrote: > On Fri, May 01, 2020 at 05:49:33PM +0200, Michał Orzeł wrote: >> >> >> On 30.04.2020 20:30, Daniel Vetter wrote: >>> On Thu, Apr 30, 2020 at 5:38 PM Sean Paul wrote: >>>> >>>> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula >>>> wrote: >>>>> >>>>> On Tue, 28 Apr 2020, Michal Orzel wrote: >>>>>> As suggested by the TODO list for the kernel DRM subsystem, replace >>>>>> the deprecated functions that take/drop modeset locks with new helpers. >>>>>> >>>>>> Signed-off-by: Michal Orzel >>>>>> --- >>>>>> drivers/gpu/drm/drm_mode_object.c | 10 ++ >>>>>> 1 file changed, 6 insertions(+), 4 deletions(-) >>>>>> >>>>>> diff --git a/drivers/gpu/drm/drm_mode_object.c >>>>>> b/drivers/gpu/drm/drm_mode_object.c >>>>>> index 35c2719..901b078 100644 >>>>>> --- a/drivers/gpu/drm/drm_mode_object.c >>>>>> +++ b/drivers/gpu/drm/drm_mode_object.c >>>>>> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct >>>>>> drm_device *dev, void *data, >>>>>> { >>>>>> struct drm_mode_obj_get_properties *arg = data; >>>>>> struct drm_mode_object *obj; >>>>>> + struct drm_modeset_acquire_ctx ctx; >>>>>> int ret = 0; >>>>>> >>>>>> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >>>>>> return -EOPNOTSUPP; >>>>>> >>>>>> - drm_modeset_lock_all(dev); >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); >>>>> >>>>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and >>>>> DRM_MODESET_LOCK_ALL_END macros. :( >>>>> >>>>> Currently only six users... but there are ~60 calls to >>>>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder >>>>> if this will come back and haunt us. >>>>> >>>> >>>> What's the alternative? Seems like the options without the macros is >>>> to use incorrect scope or have a bunch of retry/backoff cargo-cult >>>> everywhere (and hope the copy source is done correctly). >>> >>> Yeah Sean & me had a bunch of bikesheds and this is the least worst >>> option we could come up with. You can't make it a function because of >>> the control flow. You don't want to open code this because it's tricky >>> to get right, if all you want is to just grab all locks. But it is >>> magic hidden behind a macro, which occasionally ends up hurting. >>> -Daniel >> So what are we doing with this problem? Should we replace at once approx. 60 >> calls? > > I'm confused by your question - dradual conversion is entirely orthogonal > to what exactly we're converting too. All I added here is that we've > discussed this at length, and the macro is the best thing we've come up > with. I still think it's the best compromise. > > Flag-day conversion for over 60 calls doesn't work, no matter what. > -Daniel > I agree with that. All I wanted to ask was whether I should add something additional to this patch or not. Thanks, Michal >> >> Michal >>> >>>> Sean >>>> >>>>> BR, >>>>> Jani. >>>>> >>>>> >>>>>> >>>>>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, >>>>>> arg->obj_type); >>>>>> if (!obj) { >>>>>> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct >>>>>> drm_device *dev, void *data, >>>>>> out_unref: >>>>>> drm_mode_object_put(obj); >>>>>> out: >>>>>> - drm_modeset_unlock_all(dev); >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret); >>>>>> return ret; >>>>>> } >>>>>> >>>>>> @@ -449,12 +450,13 @@ static int set_property_legacy(struct >>>>>> drm_mode_object *obj, >>>>>> { >>>>>> struct drm_device *dev = prop->dev; >>>>>> struct drm_mode_object *ref; >>>>>> + struct drm_modeset_acquire_ctx ctx; >>>>>> int ret = -EINVAL; >>>>>> >>>>>> if (!drm_property_change_valid_get(prop, prop_value, &ref)) >>>>>> return -EINVAL; >>>>>> >>>>>> - drm_modeset_lock_all(dev); >>>>>> + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); >>>>>> switch (obj->type) { >>>>>> case DRM_MODE_OBJECT_CONNECTOR: >>>>>> ret = drm_connector_set_obj_prop(obj, prop, prop_value); >>>>>> @@ -468,7 +470,7 @@ static int set_property_legacy(struct >>>>>> drm_mode_object *obj, >>>>>> break; >>>>>> } >>>>>> drm_property_change_valid_put(prop, ref); >>>>>> - drm_modeset_unlock_all(dev); >>>>>> + DRM_MODESET_LOCK_ALL_END(ctx, ret); >>>>>> >>>>>> return ret; >>>>>> } >>>>> >>>>> -- >>>>> Jani Nikula, Intel Open Source Graphics Center >>> >>> >>> >
Re: [PATCH] Fix all coding-style warnings on lm75 driver
On 05.05.2020 03:58, Guenter Roeck wrote: > On Thu, Apr 30, 2020 at 04:05:34PM +0200, Michal Orzel wrote: >> Check/fix all warnings generated by checkpatch.pl script on LM75 driver. >> >> Signed-off-by: Michal Orzel > > Applied, but for the future please prepend your patches with something like > "subsystem: driver:", or for hwmon "hwmon: (driver)". > > Also, please keep in mind that such cleanups are not encouraged unless you > also provide functional changes. > > Thanks, > Guenter > I will keep it in mind. Thanks, Michal >> --- >> drivers/hwmon/lm75.c | 8 ++-- >> drivers/hwmon/lm75.h | 31 +-- >> 2 files changed, 23 insertions(+), 16 deletions(-) >> >> diff --git a/drivers/hwmon/lm75.c b/drivers/hwmon/lm75.c >> index 5e63922..ba0be48 100644 >> --- a/drivers/hwmon/lm75.c >> +++ b/drivers/hwmon/lm75.c >> @@ -797,8 +797,10 @@ static int lm75_detect(struct i2c_client *new_client, >> >> /* First check for LM75A */ >> if (i2c_smbus_read_byte_data(new_client, 7) == LM75A_ID) { >> -/* LM75A returns 0xff on unused registers so >> - just to be sure we check for that too. */ >> +/* >> + * LM75A returns 0xff on unused registers so >> + * just to be sure we check for that too. >> + */ >> if (i2c_smbus_read_byte_data(new_client, 4) != 0xff >> || i2c_smbus_read_byte_data(new_client, 5) != 0xff >> || i2c_smbus_read_byte_data(new_client, 6) != 0xff) >> @@ -849,6 +851,7 @@ static int lm75_suspend(struct device *dev) >> { >> int status; >> struct i2c_client *client = to_i2c_client(dev); >> + >> status = i2c_smbus_read_byte_data(client, LM75_REG_CONF); >> if (status < 0) { >> dev_dbg(&client->dev, "Can't read config? %d\n", status); >> @@ -863,6 +866,7 @@ static int lm75_resume(struct device *dev) >> { >> int status; >> struct i2c_client *client = to_i2c_client(dev); >> + >> status = i2c_smbus_read_byte_data(client, LM75_REG_CONF); >> if (status < 0) { >> dev_dbg(&client->dev, "Can't read config? %d\n", status); >> diff --git a/drivers/hwmon/lm75.h b/drivers/hwmon/lm75.h >> index b614e63..a398171 100644 >> --- a/drivers/hwmon/lm75.h >> +++ b/drivers/hwmon/lm75.h >> @@ -1,17 +1,15 @@ >> /* SPDX-License-Identifier: GPL-2.0-or-later */ >> /* >> -lm75.h - Part of lm_sensors, Linux kernel modules for hardware >> - monitoring >> -Copyright (c) 2003 Mark M. Hoffman >> - >> -*/ >> + * lm75.h - Part of lm_sensors, Linux kernel modules for hardware monitoring >> + * Copyright (c) 2003 Mark M. Hoffman >> + */ >> >> /* >> -This file contains common code for encoding/decoding LM75 type >> -temperature readings, which are emulated by many of the chips >> -we support. As the user is unlikely to load more than one driver >> -which contains this code, we don't worry about the wasted space. >> -*/ >> + * This file contains common code for encoding/decoding LM75 type >> + * temperature readings, which are emulated by many of the chips >> + * we support. As the user is unlikely to load more than one driver >> + * which contains this code, we don't worry about the wasted space. >> + */ >> >> #include >> >> @@ -20,18 +18,23 @@ >> #define LM75_TEMP_MAX 125000 >> #define LM75_SHUTDOWN 0x01 >> >> -/* TEMP: 0.001C/bit (-55C to +125C) >> - REG: (0.5C/bit, two's complement) << 7 */ >> +/* >> + * TEMP: 0.001C/bit (-55C to +125C) >> + * REG: (0.5C/bit, two's complement) << 7 >> + */ >> static inline u16 LM75_TEMP_TO_REG(long temp) >> { >> int ntemp = clamp_val(temp, LM75_TEMP_MIN, LM75_TEMP_MAX); >> + >> ntemp += (ntemp < 0 ? -250 : 250); >> return (u16)((ntemp / 500) << 7); >> } >> >> static inline int LM75_TEMP_FROM_REG(u16 reg) >> { >> -/* use integer division instead of equivalent right shift to >> - guarantee arithmetic shift and preserve the sign */ >> +/* >> + * use integer division instead of equivalent right shift to >> + * guarantee arithmetic shift and preserve the sign >> + */ >> return ((s16)reg / 128) * 500; >> } >> -- >> 2.7.4 >>
Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
On 30.04.2020 20:30, Daniel Vetter wrote: > On Thu, Apr 30, 2020 at 5:38 PM Sean Paul wrote: >> >> On Wed, Apr 29, 2020 at 4:57 AM Jani Nikula >> wrote: >>> >>> On Tue, 28 Apr 2020, Michal Orzel wrote: As suggested by the TODO list for the kernel DRM subsystem, replace the deprecated functions that take/drop modeset locks with new helpers. Signed-off-by: Michal Orzel --- drivers/gpu/drm/drm_mode_object.c | 10 ++ 1 file changed, 6 insertions(+), 4 deletions(-) diff --git a/drivers/gpu/drm/drm_mode_object.c b/drivers/gpu/drm/drm_mode_object.c index 35c2719..901b078 100644 --- a/drivers/gpu/drm/drm_mode_object.c +++ b/drivers/gpu/drm/drm_mode_object.c @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, { struct drm_mode_obj_get_properties *arg = data; struct drm_mode_object *obj; + struct drm_modeset_acquire_ctx ctx; int ret = 0; if (!drm_core_check_feature(dev, DRIVER_MODESET)) return -EOPNOTSUPP; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); >>> >>> I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and >>> DRM_MODESET_LOCK_ALL_END macros. :( >>> >>> Currently only six users... but there are ~60 calls to >>> drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder >>> if this will come back and haunt us. >>> >> >> What's the alternative? Seems like the options without the macros is >> to use incorrect scope or have a bunch of retry/backoff cargo-cult >> everywhere (and hope the copy source is done correctly). > > Yeah Sean & me had a bunch of bikesheds and this is the least worst > option we could come up with. You can't make it a function because of > the control flow. You don't want to open code this because it's tricky > to get right, if all you want is to just grab all locks. But it is > magic hidden behind a macro, which occasionally ends up hurting. > -Daniel So what are we doing with this problem? Should we replace at once approx. 60 calls? Michal > >> Sean >> >>> BR, >>> Jani. >>> >>> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type); if (!obj) { @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device *dev, void *data, out_unref: drm_mode_object_put(obj); out: - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(ctx, ret); return ret; } @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object *obj, { struct drm_device *dev = prop->dev; struct drm_mode_object *ref; + struct drm_modeset_acquire_ctx ctx; int ret = -EINVAL; if (!drm_property_change_valid_get(prop, prop_value, &ref)) return -EINVAL; - drm_modeset_lock_all(dev); + DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); switch (obj->type) { case DRM_MODE_OBJECT_CONNECTOR: ret = drm_connector_set_obj_prop(obj, prop, prop_value); @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object *obj, break; } drm_property_change_valid_put(prop, ref); - drm_modeset_unlock_all(dev); + DRM_MODESET_LOCK_ALL_END(ctx, ret); return ret; } >>> >>> -- >>> Jani Nikula, Intel Open Source Graphics Center > > >
Re: [PATCH] drm: Replace drm_modeset_lock/unlock_all with DRM_MODESET_LOCK_ALL_* helpers
On 29.04.2020 10:57, Jani Nikula wrote: > On Tue, 28 Apr 2020, Michal Orzel wrote: >> As suggested by the TODO list for the kernel DRM subsystem, replace >> the deprecated functions that take/drop modeset locks with new helpers. >> >> Signed-off-by: Michal Orzel >> --- >> drivers/gpu/drm/drm_mode_object.c | 10 ++ >> 1 file changed, 6 insertions(+), 4 deletions(-) >> >> diff --git a/drivers/gpu/drm/drm_mode_object.c >> b/drivers/gpu/drm/drm_mode_object.c >> index 35c2719..901b078 100644 >> --- a/drivers/gpu/drm/drm_mode_object.c >> +++ b/drivers/gpu/drm/drm_mode_object.c >> @@ -402,12 +402,13 @@ int drm_mode_obj_get_properties_ioctl(struct >> drm_device *dev, void *data, >> { >> struct drm_mode_obj_get_properties *arg = data; >> struct drm_mode_object *obj; >> +struct drm_modeset_acquire_ctx ctx; >> int ret = 0; >> >> if (!drm_core_check_feature(dev, DRIVER_MODESET)) >> return -EOPNOTSUPP; >> >> -drm_modeset_lock_all(dev); >> +DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); > > I cry a little every time I look at the DRM_MODESET_LOCK_ALL_BEGIN and > DRM_MODESET_LOCK_ALL_END macros. :( > > Currently only six users... but there are ~60 calls to > drm_modeset_lock_all{,_ctx} that I presume are to be replaced. I wonder > if this will come back and haunt us. > > BR, > Jani. Hm, so we can either replace all of these calls(I think it's a better option) or abandon the idea of removing this deprecated function. In the latter scenario, it'd be beneficial to remove this from TODO. Best regards Michal > > >> >> obj = drm_mode_object_find(dev, file_priv, arg->obj_id, arg->obj_type); >> if (!obj) { >> @@ -427,7 +428,7 @@ int drm_mode_obj_get_properties_ioctl(struct drm_device >> *dev, void *data, >> out_unref: >> drm_mode_object_put(obj); >> out: >> -drm_modeset_unlock_all(dev); >> +DRM_MODESET_LOCK_ALL_END(ctx, ret); >> return ret; >> } >> >> @@ -449,12 +450,13 @@ static int set_property_legacy(struct drm_mode_object >> *obj, >> { >> struct drm_device *dev = prop->dev; >> struct drm_mode_object *ref; >> +struct drm_modeset_acquire_ctx ctx; >> int ret = -EINVAL; >> >> if (!drm_property_change_valid_get(prop, prop_value, &ref)) >> return -EINVAL; >> >> -drm_modeset_lock_all(dev); >> +DRM_MODESET_LOCK_ALL_BEGIN(dev, ctx, 0, ret); >> switch (obj->type) { >> case DRM_MODE_OBJECT_CONNECTOR: >> ret = drm_connector_set_obj_prop(obj, prop, prop_value); >> @@ -468,7 +470,7 @@ static int set_property_legacy(struct drm_mode_object >> *obj, >> break; >> } >> drm_property_change_valid_put(prop, ref); >> -drm_modeset_unlock_all(dev); >> +DRM_MODESET_LOCK_ALL_END(ctx, ret); >> >> return ret; >> } >