Hi Heiko, On Thu, Apr 18, 2024 at 12:25:49PM +0200, Heiko Carstens wrote: > On Thu, Apr 18, 2024 at 11:54:38AM +0200, Heiko Carstens wrote: > > On Wed, Apr 17, 2024 at 11:24:35AM -0700, Nathan Chancellor wrote: > > > Clang warns (or errors with CONFIG_WERROR) after enabling > > > -Wcast-function-type-strict by default: > > > > > > drivers/s390/char/vmlogrdr.c:746:18: error: cast from 'void (*)(const void *)' to 'void (*)(struct device *)' converts to incompatible function type [-Werror,-Wcast-function-type-strict] > > > 746 | dev->release = (void (*)(struct device *))kfree; > > > | ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ > > > 1 error generated. > > > > > > Add a standalone function to fix the warning properly, which addresses > > > the root of the warning that these casts are not safe for kCFI. The > > > comment is not really relevant after this change, so remove it. > > > > > > Signed-off-by: Nathan Chancellor <nathan@xxxxxxxxxx> > > > --- > > > drivers/s390/char/vmlogrdr.c | 13 +++++-------- > > > 1 file changed, 5 insertions(+), 8 deletions(-) > > > > > @@ -736,14 +740,7 @@ static int vmlogrdr_register_device(struct vmlogrdr_priv_t *priv) > > > dev->driver = &vmlogrdr_driver; > > > dev->groups = vmlogrdr_attr_groups; > > > dev_set_drvdata(dev, priv); > > > - /* > > > - * The release function could be called after the > > > - * module has been unloaded. It's _only_ task is to > > > - * free the struct. Therefore, we specify kfree() > > > - * directly here. (Probably a little bit obfuscating > > > - * but legitime ...). > > > - */ > > > > Why is the comment not relevant after this change? Or better: why is it not > > valid before this change, which is why the code was introduced a very long > > time ago? Any reference? > > > > I've seen the warning since quite some time, but didn't change the code > > before sure that this doesn't introduce the bug described in the comment. > > From only 20 years ago: > > https://lore.kernel.org/all/20040316170812.GA14971@xxxxxxxxx/ > > The particular code (zfcp) was changed, so it doesn't have this code > (or never did?) anymore, but for the rest this may or may not still > be valid. I guess relevant may not have been the correct word. Maybe obvious? I can keep the comment but I do not really see what it adds, although reading the above thread, I suppose it was added as justification for calling kfree() as ->release() for a 'struct device'? Kind of seems like that ship has sailed since I see this all over the place as a ->release() function. I do not see how this patch could have a function change beyond that but I may be misreading or misinterpreting your full comment. Cheers, Nathan
