* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
@ 2017-08-08 11:03 Jisheng Zhang
2017-08-23 1:51 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: Jisheng Zhang @ 2017-08-08 11:03 UTC (permalink / raw)
To: linux-arm-kernel
As noted in commit d0bdff0db809 ("staging: Fix build issues with new
binder API"), we can add back the choice for 32bit ARM "once a 64bit
__get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
8091/2: add get_user() support for 8 byte types") has added the
support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
32bit ARM
Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
---
drivers/android/Kconfig | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
index 832e885349b1..aca5dc30b97b 100644
--- a/drivers/android/Kconfig
+++ b/drivers/android/Kconfig
@@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
therefore logically separated from the other devices.
config ANDROID_BINDER_IPC_32BIT
- bool
+ bool "Use old (Android 4.4 and earlier) 32-bit binder API"
depends on !64BIT && ANDROID_BINDER_IPC
default y
---help---
--
2.14.0
^ permalink raw reply related [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-08 11:03 [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM Jisheng Zhang
@ 2017-08-23 1:51 ` Greg KH
2017-08-23 2:34 ` Jisheng Zhang
0 siblings, 1 reply; 12+ messages in thread
From: Greg KH @ 2017-08-23 1:51 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> binder API"), we can add back the choice for 32bit ARM "once a 64bit
> __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> 8091/2: add get_user() support for 8 byte types") has added the
> support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> 32bit ARM
Ok, but:
>
> Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> ---
> drivers/android/Kconfig | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> index 832e885349b1..aca5dc30b97b 100644
> --- a/drivers/android/Kconfig
> +++ b/drivers/android/Kconfig
> @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> therefore logically separated from the other devices.
>
> config ANDROID_BINDER_IPC_32BIT
> - bool
> + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> depends on !64BIT && ANDROID_BINDER_IPC
You don't actually change the depends line :(
Please fix up, and test it, and then resend.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 1:51 ` Greg KH
@ 2017-08-23 2:34 ` Jisheng Zhang
2017-08-23 2:56 ` John Stultz
2017-08-23 5:35 ` Greg KH
0 siblings, 2 replies; 12+ messages in thread
From: Jisheng Zhang @ 2017-08-23 2:34 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> > 8091/2: add get_user() support for 8 byte types") has added the
> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> > 32bit ARM
>
> Ok, but:
>
> >
> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > ---
> > drivers/android/Kconfig | 2 +-
> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> > index 832e885349b1..aca5dc30b97b 100644
> > --- a/drivers/android/Kconfig
> > +++ b/drivers/android/Kconfig
> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> > therefore logically separated from the other devices.
> >
> > config ANDROID_BINDER_IPC_32BIT
> > - bool
> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> > depends on !64BIT && ANDROID_BINDER_IPC
>
> You don't actually change the depends line :(
>
> Please fix up, and test it, and then resend.
IHOM, the dependency is correct: 64bit platforms don't support
ANDROID_BINDER_IPC_32BIT. What do you think?
Thanks,
Jisheng
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:34 ` Jisheng Zhang
@ 2017-08-23 2:56 ` John Stultz
2017-08-23 2:56 ` [EXT] " Jisheng Zhang
2017-08-23 2:57 ` John Stultz
2017-08-23 5:35 ` Greg KH
1 sibling, 2 replies; 12+ messages in thread
From: John Stultz @ 2017-08-23 2:56 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
>
>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
>> > 8091/2: add get_user() support for 8 byte types") has added the
>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
>> > 32bit ARM
>>
>> Ok, but:
>>
>> >
>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> > ---
>> > drivers/android/Kconfig | 2 +-
>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
>> > index 832e885349b1..aca5dc30b97b 100644
>> > --- a/drivers/android/Kconfig
>> > +++ b/drivers/android/Kconfig
>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
>> > therefore logically separated from the other devices.
>> >
>> > config ANDROID_BINDER_IPC_32BIT
>> > - bool
>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
>> > depends on !64BIT && ANDROID_BINDER_IPC
>>
>> You don't actually change the depends line :(
>>
>> Please fix up, and test it, and then resend.
>
> IHOM, the dependency is correct: 64bit platforms don't support
> ANDROID_BINDER_IPC_32BIT. What do you think?
I think this indicates the commit message is unclear.
Part of it is that the config is inverted from the description. The
patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
just allows the option to be unselected, so that the 64bit ABI will be
used on 32bit systems.
Conceptually I don't have an objection to the change (though maybe try
to rework the commit message), but I don't have anything to actually
test it on right now, so I'm hesitant to ack it.
thanks
-john
^ permalink raw reply [flat|nested] 12+ messages in thread
* [EXT] Re: [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:56 ` John Stultz
@ 2017-08-23 2:56 ` Jisheng Zhang
2017-08-23 2:57 ` John Stultz
1 sibling, 0 replies; 12+ messages in thread
From: Jisheng Zhang @ 2017-08-23 2:56 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 22 Aug 2017 19:56:07 -0700 John Stultz wrote:
> External Email
>
> ----------------------------------------------------------------------
> On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> > On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
> >
> >> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> >> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> >> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> >> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> >> > 8091/2: add get_user() support for 8 byte types") has added the
> >> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> >> > 32bit ARM
> >>
> >> Ok, but:
> >>
> >> >
> >> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >> > ---
> >> > drivers/android/Kconfig | 2 +-
> >> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >
> >> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> >> > index 832e885349b1..aca5dc30b97b 100644
> >> > --- a/drivers/android/Kconfig
> >> > +++ b/drivers/android/Kconfig
> >> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> >> > therefore logically separated from the other devices.
> >> >
> >> > config ANDROID_BINDER_IPC_32BIT
> >> > - bool
> >> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> >> > depends on !64BIT && ANDROID_BINDER_IPC
> >>
> >> You don't actually change the depends line :(
> >>
> >> Please fix up, and test it, and then resend.
> >
> > IHOM, the dependency is correct: 64bit platforms don't support
> > ANDROID_BINDER_IPC_32BIT. What do you think?
>
> I think this indicates the commit message is unclear.
>
> Part of it is that the config is inverted from the description. The
> patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
> just allows the option to be unselected, so that the 64bit ABI will be
> used on 32bit systems.
>
> Conceptually I don't have an objection to the change (though maybe try
> to rework the commit message), but I don't have anything to actually
> test it on right now, so I'm hesitant to ack it.
>
I have tested it on Marvell Berlin 32bit platform: the 64bit binder IPC
works fine on our 32bit platform.
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:56 ` John Stultz
2017-08-23 2:56 ` [EXT] " Jisheng Zhang
@ 2017-08-23 2:57 ` John Stultz
2017-08-23 3:01 ` Jisheng Zhang
2017-08-23 18:48 ` Rob Herring
1 sibling, 2 replies; 12+ messages in thread
From: John Stultz @ 2017-08-23 2:57 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
>>
>>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
>>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
>>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
>>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
>>> > 8091/2: add get_user() support for 8 byte types") has added the
>>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
>>> > 32bit ARM
>>>
>>> Ok, but:
>>>
>>> >
>>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>> > ---
>>> > drivers/android/Kconfig | 2 +-
>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>> >
>>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
>>> > index 832e885349b1..aca5dc30b97b 100644
>>> > --- a/drivers/android/Kconfig
>>> > +++ b/drivers/android/Kconfig
>>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
>>> > therefore logically separated from the other devices.
>>> >
>>> > config ANDROID_BINDER_IPC_32BIT
>>> > - bool
>>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
>>> > depends on !64BIT && ANDROID_BINDER_IPC
>>>
>>> You don't actually change the depends line :(
>>>
>>> Please fix up, and test it, and then resend.
>>
>> IHOM, the dependency is correct: 64bit platforms don't support
>> ANDROID_BINDER_IPC_32BIT. What do you think?
>
> I think this indicates the commit message is unclear.
>
> Part of it is that the config is inverted from the description. The
> patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
> just allows the option to be unselected, so that the 64bit ABI will be
> used on 32bit systems.
>
> Conceptually I don't have an objection to the change (though maybe try
> to rework the commit message), but I don't have anything to actually
> test it on right now, so I'm hesitant to ack it.
It might also be good to add some detail as to the motivation for this
change? What benefit does it bring to 32bit platforms to use the newer
64bit ABI?
thanks
-john
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:57 ` John Stultz
@ 2017-08-23 3:01 ` Jisheng Zhang
2017-08-23 3:14 ` John Stultz
2017-08-23 18:48 ` Rob Herring
1 sibling, 1 reply; 12+ messages in thread
From: Jisheng Zhang @ 2017-08-23 3:01 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, 22 Aug 2017 19:57:04 -0700 John Stultz wrote:
> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> >> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
> >>
> >>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> >>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> >>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> >>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> >>> > 8091/2: add get_user() support for 8 byte types") has added the
> >>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> >>> > 32bit ARM
> >>>
> >>> Ok, but:
> >>>
> >>> >
> >>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>> > ---
> >>> > drivers/android/Kconfig | 2 +-
> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >>> >
> >>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> >>> > index 832e885349b1..aca5dc30b97b 100644
> >>> > --- a/drivers/android/Kconfig
> >>> > +++ b/drivers/android/Kconfig
> >>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> >>> > therefore logically separated from the other devices.
> >>> >
> >>> > config ANDROID_BINDER_IPC_32BIT
> >>> > - bool
> >>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> >>> > depends on !64BIT && ANDROID_BINDER_IPC
> >>>
> >>> You don't actually change the depends line :(
> >>>
> >>> Please fix up, and test it, and then resend.
> >>
> >> IHOM, the dependency is correct: 64bit platforms don't support
> >> ANDROID_BINDER_IPC_32BIT. What do you think?
> >
> > I think this indicates the commit message is unclear.
> >
> > Part of it is that the config is inverted from the description. The
> > patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
> > just allows the option to be unselected, so that the 64bit ABI will be
> > used on 32bit systems.
> >
> > Conceptually I don't have an objection to the change (though maybe try
> > to rework the commit message), but I don't have anything to actually
> > test it on right now, so I'm hesitant to ack it.
>
> It might also be good to add some detail as to the motivation for this
> change? What benefit does it bring to 32bit platforms to use the newer
> 64bit ABI?
>
To be honest, the motivation is just to add one more choice for 32bit
platform and let the code be tested under 32bit platform. Maybe we
could then remove ANDROID_BINDER_IPC_32BIT and the related code after
some time?
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 3:01 ` Jisheng Zhang
@ 2017-08-23 3:14 ` John Stultz
2017-08-23 5:37 ` Greg KH
0 siblings, 1 reply; 12+ messages in thread
From: John Stultz @ 2017-08-23 3:14 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 22, 2017 at 8:01 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> On Tue, 22 Aug 2017 19:57:04 -0700 John Stultz wrote:
>
>> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
>> > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>> >> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
>> >>
>> >>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
>> >>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
>> >>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
>> >>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
>> >>> > 8091/2: add get_user() support for 8 byte types") has added the
>> >>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
>> >>> > 32bit ARM
>> >>>
>> >>> Ok, but:
>> >>>
>> >>> >
>> >>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>> >>> > ---
>> >>> > drivers/android/Kconfig | 2 +-
>> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>> >>> >
>> >>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
>> >>> > index 832e885349b1..aca5dc30b97b 100644
>> >>> > --- a/drivers/android/Kconfig
>> >>> > +++ b/drivers/android/Kconfig
>> >>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
>> >>> > therefore logically separated from the other devices.
>> >>> >
>> >>> > config ANDROID_BINDER_IPC_32BIT
>> >>> > - bool
>> >>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
>> >>> > depends on !64BIT && ANDROID_BINDER_IPC
>> >>>
>> >>> You don't actually change the depends line :(
>> >>>
>> >>> Please fix up, and test it, and then resend.
>> >>
>> >> IHOM, the dependency is correct: 64bit platforms don't support
>> >> ANDROID_BINDER_IPC_32BIT. What do you think?
>> >
>> > I think this indicates the commit message is unclear.
>> >
>> > Part of it is that the config is inverted from the description. The
>> > patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
>> > just allows the option to be unselected, so that the 64bit ABI will be
>> > used on 32bit systems.
>> >
>> > Conceptually I don't have an objection to the change (though maybe try
>> > to rework the commit message), but I don't have anything to actually
>> > test it on right now, so I'm hesitant to ack it.
>>
>> It might also be good to add some detail as to the motivation for this
>> change? What benefit does it bring to 32bit platforms to use the newer
>> 64bit ABI?
>>
>
> To be honest, the motivation is just to add one more choice for 32bit
> platform and let the code be tested under 32bit platform. Maybe we
> could then remove ANDROID_BINDER_IPC_32BIT and the related code after
> some time?
I'm mixed. It would be nice to deprecate the old 32bit ABI, but binder
is a real Linux kernel interface now, so we don't break compatibility
(at least if it affects anyone - which may be questionable here - not
sure there's many upstream 32bit platforms concerned with running
legacy Android builds). But just adding the extra option just means
there's yet another configuration to test and to keep working. So you
may want to articulate the benefits better to make this worth the
effort of doing a full transition.
thanks
-john
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:34 ` Jisheng Zhang
2017-08-23 2:56 ` John Stultz
@ 2017-08-23 5:35 ` Greg KH
1 sibling, 0 replies; 12+ messages in thread
From: Greg KH @ 2017-08-23 5:35 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, Aug 23, 2017 at 10:34:20AM +0800, Jisheng Zhang wrote:
> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
>
> > On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> > > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> > > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> > > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> > > 8091/2: add get_user() support for 8 byte types") has added the
> > > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> > > 32bit ARM
> >
> > Ok, but:
> >
> > >
> > > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> > > ---
> > > drivers/android/Kconfig | 2 +-
> > > 1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> > > index 832e885349b1..aca5dc30b97b 100644
> > > --- a/drivers/android/Kconfig
> > > +++ b/drivers/android/Kconfig
> > > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> > > therefore logically separated from the other devices.
> > >
> > > config ANDROID_BINDER_IPC_32BIT
> > > - bool
> > > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> > > depends on !64BIT && ANDROID_BINDER_IPC
> >
> > You don't actually change the depends line :(
> >
> > Please fix up, and test it, and then resend.
>
> IHOM, the dependency is correct: 64bit platforms don't support
> ANDROID_BINDER_IPC_32BIT. What do you think?
I think your changelog text is totally wrong, please fix this up
properly and resend it if you think it is needed.
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 3:14 ` John Stultz
@ 2017-08-23 5:37 ` Greg KH
0 siblings, 0 replies; 12+ messages in thread
From: Greg KH @ 2017-08-23 5:37 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 22, 2017 at 08:14:10PM -0700, John Stultz wrote:
> On Tue, Aug 22, 2017 at 8:01 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> > On Tue, 22 Aug 2017 19:57:04 -0700 John Stultz wrote:
> >
> >> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> > On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> >> >> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
> >> >>
> >> >>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> >> >>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> >> >>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> >> >>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> >> >>> > 8091/2: add get_user() support for 8 byte types") has added the
> >> >>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> >> >>> > 32bit ARM
> >> >>>
> >> >>> Ok, but:
> >> >>>
> >> >>> >
> >> >>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >> >>> > ---
> >> >>> > drivers/android/Kconfig | 2 +-
> >> >>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >> >>> >
> >> >>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> >> >>> > index 832e885349b1..aca5dc30b97b 100644
> >> >>> > --- a/drivers/android/Kconfig
> >> >>> > +++ b/drivers/android/Kconfig
> >> >>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> >> >>> > therefore logically separated from the other devices.
> >> >>> >
> >> >>> > config ANDROID_BINDER_IPC_32BIT
> >> >>> > - bool
> >> >>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> >> >>> > depends on !64BIT && ANDROID_BINDER_IPC
> >> >>>
> >> >>> You don't actually change the depends line :(
> >> >>>
> >> >>> Please fix up, and test it, and then resend.
> >> >>
> >> >> IHOM, the dependency is correct: 64bit platforms don't support
> >> >> ANDROID_BINDER_IPC_32BIT. What do you think?
> >> >
> >> > I think this indicates the commit message is unclear.
> >> >
> >> > Part of it is that the config is inverted from the description. The
> >> > patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
> >> > just allows the option to be unselected, so that the 64bit ABI will be
> >> > used on 32bit systems.
> >> >
> >> > Conceptually I don't have an objection to the change (though maybe try
> >> > to rework the commit message), but I don't have anything to actually
> >> > test it on right now, so I'm hesitant to ack it.
> >>
> >> It might also be good to add some detail as to the motivation for this
> >> change? What benefit does it bring to 32bit platforms to use the newer
> >> 64bit ABI?
> >>
> >
> > To be honest, the motivation is just to add one more choice for 32bit
> > platform and let the code be tested under 32bit platform. Maybe we
> > could then remove ANDROID_BINDER_IPC_32BIT and the related code after
> > some time?
>
> I'm mixed. It would be nice to deprecate the old 32bit ABI, but binder
> is a real Linux kernel interface now, so we don't break compatibility
> (at least if it affects anyone - which may be questionable here - not
> sure there's many upstream 32bit platforms concerned with running
> legacy Android builds). But just adding the extra option just means
> there's yet another configuration to test and to keep working. So you
> may want to articulate the benefits better to make this worth the
> effort of doing a full transition.
Future versions of Android "might" be changing if they support, or not,
all of the different combinations of 32/64 bit apis. So maybe we should
wait until a real solid direction with regards to this is published
before worrying about dropping anything...
thanks,
greg k-h
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 2:57 ` John Stultz
2017-08-23 3:01 ` Jisheng Zhang
@ 2017-08-23 18:48 ` Rob Herring
2017-08-24 1:44 ` Jisheng Zhang
1 sibling, 1 reply; 12+ messages in thread
From: Rob Herring @ 2017-08-23 18:48 UTC (permalink / raw)
To: linux-arm-kernel
On Tue, Aug 22, 2017 at 9:57 PM, John Stultz <john.stultz@linaro.org> wrote:
> On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
>> On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
>>> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
>>>
>>>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
>>>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
>>>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
>>>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
>>>> > 8091/2: add get_user() support for 8 byte types") has added the
>>>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
>>>> > 32bit ARM
>>>>
>>>> Ok, but:
>>>>
>>>> >
>>>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
>>>> > ---
>>>> > drivers/android/Kconfig | 2 +-
>>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
>>>> >
>>>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
>>>> > index 832e885349b1..aca5dc30b97b 100644
>>>> > --- a/drivers/android/Kconfig
>>>> > +++ b/drivers/android/Kconfig
>>>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
>>>> > therefore logically separated from the other devices.
>>>> >
>>>> > config ANDROID_BINDER_IPC_32BIT
>>>> > - bool
>>>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
>>>> > depends on !64BIT && ANDROID_BINDER_IPC
>>>>
>>>> You don't actually change the depends line :(
>>>>
>>>> Please fix up, and test it, and then resend.
>>>
>>> IHOM, the dependency is correct: 64bit platforms don't support
>>> ANDROID_BINDER_IPC_32BIT. What do you think?
>>
>> I think this indicates the commit message is unclear.
>>
>> Part of it is that the config is inverted from the description. The
>> patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
>> just allows the option to be unselected, so that the 64bit ABI will be
>> used on 32bit systems.
>>
>> Conceptually I don't have an objection to the change (though maybe try
>> to rework the commit message), but I don't have anything to actually
>> test it on right now, so I'm hesitant to ack it.
>
> It might also be good to add some detail as to the motivation for this
> change? What benefit does it bring to 32bit platforms to use the newer
> 64bit ABI?
It allows running the same 32-bit userspace build whether the kernel
is 64-bit or 32-bit.
Rob
^ permalink raw reply [flat|nested] 12+ messages in thread
* [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM
2017-08-23 18:48 ` Rob Herring
@ 2017-08-24 1:44 ` Jisheng Zhang
0 siblings, 0 replies; 12+ messages in thread
From: Jisheng Zhang @ 2017-08-24 1:44 UTC (permalink / raw)
To: linux-arm-kernel
On Wed, 23 Aug 2017 13:48:47 -0500 Rob Herring wrote:
> On Tue, Aug 22, 2017 at 9:57 PM, John Stultz <john.stultz@linaro.org> wrote:
> > On Tue, Aug 22, 2017 at 7:56 PM, John Stultz <john.stultz@linaro.org> wrote:
> >> On Tue, Aug 22, 2017 at 7:34 PM, Jisheng Zhang <jszhang@marvell.com> wrote:
> >>> On Tue, 22 Aug 2017 18:51:08 -0700 Greg KH wrote:
> >>>
> >>>> On Tue, Aug 08, 2017 at 07:03:05PM +0800, Jisheng Zhang wrote:
> >>>> > As noted in commit d0bdff0db809 ("staging: Fix build issues with new
> >>>> > binder API"), we can add back the choice for 32bit ARM "once a 64bit
> >>>> > __get_user_asm_* implementation is merged." Commit e38361d032f1 ("ARM:
> >>>> > 8091/2: add get_user() support for 8 byte types") has added the
> >>>> > support, so it's time to let ANDROID_BINDER_IPC_32BIT be selectable on
> >>>> > 32bit ARM
> >>>>
> >>>> Ok, but:
> >>>>
> >>>> >
> >>>> > Signed-off-by: Jisheng Zhang <jszhang@marvell.com>
> >>>> > ---
> >>>> > drivers/android/Kconfig | 2 +-
> >>>> > 1 file changed, 1 insertion(+), 1 deletion(-)
> >>>> >
> >>>> > diff --git a/drivers/android/Kconfig b/drivers/android/Kconfig
> >>>> > index 832e885349b1..aca5dc30b97b 100644
> >>>> > --- a/drivers/android/Kconfig
> >>>> > +++ b/drivers/android/Kconfig
> >>>> > @@ -32,7 +32,7 @@ config ANDROID_BINDER_DEVICES
> >>>> > therefore logically separated from the other devices.
> >>>> >
> >>>> > config ANDROID_BINDER_IPC_32BIT
> >>>> > - bool
> >>>> > + bool "Use old (Android 4.4 and earlier) 32-bit binder API"
> >>>> > depends on !64BIT && ANDROID_BINDER_IPC
> >>>>
> >>>> You don't actually change the depends line :(
> >>>>
> >>>> Please fix up, and test it, and then resend.
> >>>
> >>> IHOM, the dependency is correct: 64bit platforms don't support
> >>> ANDROID_BINDER_IPC_32BIT. What do you think?
> >>
> >> I think this indicates the commit message is unclear.
> >>
> >> Part of it is that the config is inverted from the description. The
> >> patch doesn't enable the 32bit legacy binder ABI on 32bit systems, it
> >> just allows the option to be unselected, so that the 64bit ABI will be
> >> used on 32bit systems.
> >>
> >> Conceptually I don't have an objection to the change (though maybe try
> >> to rework the commit message), but I don't have anything to actually
> >> test it on right now, so I'm hesitant to ack it.
> >
> > It might also be good to add some detail as to the motivation for this
> > change? What benefit does it bring to 32bit platforms to use the newer
> > 64bit ABI?
>
> It allows running the same 32-bit userspace build whether the kernel
> is 64-bit or 32-bit.
>
Oh, yes, this is a good "motivation" which can be added into the commit msg.
thanks for the inspiration.
^ permalink raw reply [flat|nested] 12+ messages in thread
end of thread, other threads:[~2017-08-24 1:44 UTC | newest]
Thread overview: 12+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2017-08-08 11:03 [PATCH] binder: let ANDROID_BINDER_IPC_32BIT be selectable on 32bit ARM Jisheng Zhang
2017-08-23 1:51 ` Greg KH
2017-08-23 2:34 ` Jisheng Zhang
2017-08-23 2:56 ` John Stultz
2017-08-23 2:56 ` [EXT] " Jisheng Zhang
2017-08-23 2:57 ` John Stultz
2017-08-23 3:01 ` Jisheng Zhang
2017-08-23 3:14 ` John Stultz
2017-08-23 5:37 ` Greg KH
2017-08-23 18:48 ` Rob Herring
2017-08-24 1:44 ` Jisheng Zhang
2017-08-23 5:35 ` Greg KH
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).