Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On Thu, 05 Nov 2020, Paul Fulghum wrote: > > Another candidate for removal is drivers/char/pcmcia/synclink_cs.c > > Everything I said about synclink.c/synclinkmp.c is true of that as well: the > hardware stopped being manufactured decades ago and is not available for > testing. The possibility of these cards still being around/functional for use > with the latest kernel is about zero. > > If Lee Jones does wants to add that to his patch, great. If not then I can do > so. I'll probably send this as a follow-up, as it's easier to get merged into a different subsystem if they patches are separate and orthogonal. Will submit them on the morrow. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
Another candidate for removal is drivers/char/pcmcia/synclink_cs.c Everything I said about synclink.c/synclinkmp.c is true of that as well: the hardware stopped being manufactured decades ago and is not available for testing. The possibility of these cards still being around/functional for use with the latest kernel is about zero. If Lee Jones does wants to add that to his patch, great. If not then I can do so. (I resent this message in plain text after it was rejected for HTML, sorry if anyone got a duplicate.) > On Nov 5, 2020, at 3:27 AM, Lee Jones wrote: > > Will post the removal patches when my tests finish.
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On 05. 11. 20, 12:04, David Laight wrote: And the loop can be turned into ndelay: /* * Force at least 170ns delay before clearing * reset bit. Each read from LCR takes at least * 30ns so 10 times for 300ns to be safe. */ for(i=0;i<10;i++) readval = *MiscCtrl; Again, since I can't test this, I do not want this patch to contain any functional changes. AFAIC, the 10 register reads must still happen after this patch is applied. You can't use ndelay(); the writes can get posted so can appear much closer together by the time they get to the actual hardware. Ah, indeed, this is on PCI. One read would need to stay before ndelay then. But given it completely goes away, no need to worry. Thanks for heads up. -- js suse labs
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On Thu, 05 Nov 2020, David Laight wrote: > > >> And the loop can be turned into ndelay: > > >> > > >> /* > > >> * Force at least 170ns delay before clearing > > >> * reset bit. Each read from LCR takes at least > > >> * 30ns so 10 times for 300ns to be safe. > > >> */ > > >> for(i=0;i<10;i++) > > >> readval = *MiscCtrl; > > > > > > Again, since I can't test this, I do not want this patch to contain > > > any functional changes. AFAIC, the 10 register reads must still > > > happen after this patch is applied. > > You can't use ndelay(); the writes can get posted so can appear > much closer together by the time they get to the actual hardware. > Multiple volatile reads don't need a target variable. > (Shouldn't they also real readl()?) > > Deleting the driver works... Will post the removal patches when my tests finish. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
RE: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
> >> And the loop can be turned into ndelay: > >> > >> /* > >> * Force at least 170ns delay before clearing > >> * reset bit. Each read from LCR takes at least > >> * 30ns so 10 times for 300ns to be safe. > >> */ > >> for(i=0;i<10;i++) > >> readval = *MiscCtrl; > > > > Again, since I can't test this, I do not want this patch to contain > > any functional changes. AFAIC, the 10 register reads must still > > happen after this patch is applied. You can't use ndelay(); the writes can get posted so can appear much closer together by the time they get to the actual hardware. Multiple volatile reads don't need a target variable. (Shouldn't they also real readl()?) Deleting the driver works... David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On 05. 11. 20, 10:39, Paul Fulghum wrote: If there are no objections to removing the the old drivers (synclink.c/synclink_mp.c) I could make a patch to do so tomorrow (it is 1:30am here now). Nothing eliminates niggling warnings like removing dead code. \o/ Thanks a lot. -- js suse labs
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On Thu, 05 Nov 2020, Greg Kroah-Hartman wrote: > On Thu, Nov 05, 2020 at 01:39:36AM -0800, Paul Fulghum wrote: > > > > > > > On Nov 5, 2020, at 1:10 AM, Jiri Slaby wrote: > > > > > > OK, let the loop alone. I would bet a half a pig that noone is able to > > > test this driver. But one has to write this for someone to raise and > > > admit they are still using it. In fact, there are _4_ google replies to > > > "Microgate Corporation" "SyncLink Multiport Adapter" "lspci". > > > > > > > > The hardware used with synclink.c and synclinkmp.c has not been > manufactured for 15 years and was low volume. The chances of either > driver still being in use is very low. Not even Microgate (me) has > the ability to test either anymore (no hardware). I don’t know the > policy about driver removal, but I think both could be removed > without upsetting anyone. > > > > synclink_gt.c is still in production and the driver still actively used. > > > > If there are no objections to removing the the old drivers > > (synclink.c/synclink_mp.c) I could make a patch to do so tomorrow > > (it is 1:30am here now). Nothing eliminates niggling warnings like > > removing dead code. > > Great, please submit a patch to remove these, I will always take patches > to delete lines :) Good resolution. I'm happy to do this. Bear with me. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On Thu, Nov 05, 2020 at 01:39:36AM -0800, Paul Fulghum wrote: > > > > On Nov 5, 2020, at 1:10 AM, Jiri Slaby wrote: > > > > OK, let the loop alone. I would bet a half a pig that noone is able to test > > this driver. But one has to write this for someone to raise and admit they > > are still using it. In fact, there are _4_ google replies to "Microgate > > Corporation" "SyncLink Multiport Adapter" "lspci". > > > > The hardware used with synclink.c and synclinkmp.c has not been manufactured > for 15 years and was low volume. The chances of either driver still being in > use is very low. Not even Microgate (me) has the ability to test either > anymore (no hardware). I don’t know the policy about driver removal, but I > think both could be removed without upsetting anyone. > > synclink_gt.c is still in production and the driver still actively used. > > If there are no objections to removing the the old drivers > (synclink.c/synclink_mp.c) I could make a patch to do so tomorrow (it is > 1:30am here now). Nothing eliminates niggling warnings like removing dead > code. Great, please submit a patch to remove these, I will always take patches to delete lines :) thanks, greg k-h
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
> On Nov 5, 2020, at 1:10 AM, Jiri Slaby wrote: > > OK, let the loop alone. I would bet a half a pig that noone is able to test > this driver. But one has to write this for someone to raise and admit they > are still using it. In fact, there are _4_ google replies to "Microgate > Corporation" "SyncLink Multiport Adapter" "lspci". The hardware used with synclink.c and synclinkmp.c has not been manufactured for 15 years and was low volume. The chances of either driver still being in use is very low. Not even Microgate (me) has the ability to test either anymore (no hardware). I don’t know the policy about driver removal, but I think both could be removed without upsetting anyone. synclink_gt.c is still in production and the driver still actively used. If there are no objections to removing the the old drivers (synclink.c/synclink_mp.c) I could make a patch to do so tomorrow (it is 1:30am here now). Nothing eliminates niggling warnings like removing dead code.
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On 05. 11. 20, 9:43, Lee Jones wrote: On Thu, 05 Nov 2020, Jiri Slaby wrote: On 04. 11. 20, 20:35, Lee Jones wrote: Fixes the following W=1 kernel build warning(s): drivers/tty/synclinkmp.c: In function ‘init_adapter’: drivers/tty/synclinkmp.c:5167:6: warning: variable ‘readval’ set but not used [-Wunused-but-set-variable] Cc: Greg Kroah-Hartman Cc: Jiri Slaby Cc: pau...@microgate.com Signed-off-by: Lee Jones --- drivers/tty/synclinkmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c index 0ca738f61a35b..75f494bfdcbed 100644 --- a/drivers/tty/synclinkmp.c +++ b/drivers/tty/synclinkmp.c @@ -5165,7 +5165,7 @@ static bool init_adapter(SLMP_INFO *info) /* Set BIT30 of Local Control Reg 0x50 to reset SCA */ volatile u32 *MiscCtrl = (u32 *)(info->lcr_base + 0x50); - u32 readval; + u32 __always_unused readval; Why not just remove readval completely as in other cases? Because I don't know what the result would be. Will the read still happen, or will the compiler optimise it away? The compiler will eliminate this dead store anyway. However given the MiscCtrl pointer is volatile, the read proper must remain. My changes should not affect any of the instructions i.e. the register read must still take place I understand. But the C specification helps here. And the loop can be turned into ndelay: /* * Force at least 170ns delay before clearing * reset bit. Each read from LCR takes at least * 30ns so 10 times for 300ns to be safe. */ for(i=0;i<10;i++) readval = *MiscCtrl; Again, since I can't test this, I do not want this patch to contain any functional changes. AFAIC, the 10 register reads must still happen after this patch is applied. OK, let the loop alone. I would bet a half a pig that noone is able to test this driver. But one has to write this for someone to raise and admit they are still using it. In fact, there are _4_ google replies to "Microgate Corporation" "SyncLink Multiport Adapter" "lspci". thanks, -- js suse labs
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On Thu, 05 Nov 2020, Jiri Slaby wrote: > On 04. 11. 20, 20:35, Lee Jones wrote: > > Fixes the following W=1 kernel build warning(s): > > > > drivers/tty/synclinkmp.c: In function ‘init_adapter’: > > drivers/tty/synclinkmp.c:5167:6: warning: variable ‘readval’ set but not > > used [-Wunused-but-set-variable] > > > > Cc: Greg Kroah-Hartman > > Cc: Jiri Slaby > > Cc: pau...@microgate.com > > Signed-off-by: Lee Jones > > --- > > drivers/tty/synclinkmp.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c > > index 0ca738f61a35b..75f494bfdcbed 100644 > > --- a/drivers/tty/synclinkmp.c > > +++ b/drivers/tty/synclinkmp.c > > @@ -5165,7 +5165,7 @@ static bool init_adapter(SLMP_INFO *info) > > /* Set BIT30 of Local Control Reg 0x50 to reset SCA */ > > volatile u32 *MiscCtrl = (u32 *)(info->lcr_base + 0x50); > > - u32 readval; > > + u32 __always_unused readval; > > Why not just remove readval completely as in other cases? Because I don't know what the result would be. Will the read still happen, or will the compiler optimise it away? My changes should not affect any of the instructions i.e. the register read must still take place > And the loop can be turned into ndelay: > > /* > * Force at least 170ns delay before clearing > * reset bit. Each read from LCR takes at least > * 30ns so 10 times for 300ns to be safe. > */ > for(i=0;i<10;i++) > readval = *MiscCtrl; Again, since I can't test this, I do not want this patch to contain any functional changes. AFAIC, the 10 register reads must still happen after this patch is applied. -- Lee Jones [李琼斯] Senior Technical Lead - Developer Services Linaro.org │ Open source software for Arm SoCs Follow Linaro: Facebook | Twitter | Blog
Re: [PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
On 04. 11. 20, 20:35, Lee Jones wrote: Fixes the following W=1 kernel build warning(s): drivers/tty/synclinkmp.c: In function ‘init_adapter’: drivers/tty/synclinkmp.c:5167:6: warning: variable ‘readval’ set but not used [-Wunused-but-set-variable] Cc: Greg Kroah-Hartman Cc: Jiri Slaby Cc: pau...@microgate.com Signed-off-by: Lee Jones --- drivers/tty/synclinkmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c index 0ca738f61a35b..75f494bfdcbed 100644 --- a/drivers/tty/synclinkmp.c +++ b/drivers/tty/synclinkmp.c @@ -5165,7 +5165,7 @@ static bool init_adapter(SLMP_INFO *info) /* Set BIT30 of Local Control Reg 0x50 to reset SCA */ volatile u32 *MiscCtrl = (u32 *)(info->lcr_base + 0x50); - u32 readval; + u32 __always_unused readval; Why not just remove readval completely as in other cases? And the loop can be turned into ndelay: /* * Force at least 170ns delay before clearing * reset bit. Each read from LCR takes at least * 30ns so 10 times for 300ns to be safe. */ for(i=0;i<10;i++) readval = *MiscCtrl; thanks, -- js suse labs
[PATCH 27/36] tty: synclinkmp: Mark never checked 'readval' as __always_unused
Fixes the following W=1 kernel build warning(s): drivers/tty/synclinkmp.c: In function ‘init_adapter’: drivers/tty/synclinkmp.c:5167:6: warning: variable ‘readval’ set but not used [-Wunused-but-set-variable] Cc: Greg Kroah-Hartman Cc: Jiri Slaby Cc: pau...@microgate.com Signed-off-by: Lee Jones --- drivers/tty/synclinkmp.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/tty/synclinkmp.c b/drivers/tty/synclinkmp.c index 0ca738f61a35b..75f494bfdcbed 100644 --- a/drivers/tty/synclinkmp.c +++ b/drivers/tty/synclinkmp.c @@ -5165,7 +5165,7 @@ static bool init_adapter(SLMP_INFO *info) /* Set BIT30 of Local Control Reg 0x50 to reset SCA */ volatile u32 *MiscCtrl = (u32 *)(info->lcr_base + 0x50); - u32 readval; + u32 __always_unused readval; info->misc_ctrl_value |= BIT30; *MiscCtrl = info->misc_ctrl_value; -- 2.25.1
