| Message ID | c8bfa4da-1bcf-bd13-9bd3-b1df25730215@denx.de |
|---|---|
| State | Accepted |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | [U-Boot,PULL] u-boot-usb/master | expand |
Hello Marek, Lukasz cc: Sam On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: [..] > Sam Protsenko (3): > fastboot: Fix slot names reported by getvar Commit [1] from this PR got replaced by series [2]. If this PR is merged as-is, the benefit of the new series will be lost and the series will need rework (since it overlaps with [1]). Ideally [2] should replace commit [1]. Is this feasible and do you have any suggestions how to achieve this? TIA. [1] https://git.denx.de/?p=u-boot/u-boot-usb.git;a=commit;h=97a0c6ff577d57f162abc696c4efc962981229b1 [2] https://patchwork.ozlabs.org/cover/1116097/ ("fastboot: Support new A/B slot format") Thanks, Eugeniu,
On 6/15/19 11:46 AM, Eugeniu Rosca wrote: > Hello Marek, Lukasz cc: Sam > > On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: > [..] >> Sam Protsenko (3): >> fastboot: Fix slot names reported by getvar > > Commit [1] from this PR got replaced by series [2]. Replaced where ? [1] seems like a fix to me, [2] seems like feature addition , so merging [1] this late in the release cycle seems like the right thing to do. > If this PR is merged as-is, the benefit of the new series will be lost > and the series will need rework (since it overlaps with [1]). > Ideally [2] should replace commit [1]. Is this feasible and do you > have any suggestions how to achieve this? TIA. > > [1] https://git.denx.de/?p=u-boot/u-boot-usb.git;a=commit;h=97a0c6ff577d57f162abc696c4efc962981229b1 > [2] https://patchwork.ozlabs.org/cover/1116097/ ("fastboot: Support > new A/B slot format") > > Thanks, > Eugeniu, >
Hi Marek, On Sat, Jun 15, 2019 at 02:35:12PM +0200, Marek Vasut wrote: > On 6/15/19 11:46 AM, Eugeniu Rosca wrote: > > Hello Marek, Lukasz cc: Sam > > > > On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: > > [..] > >> Sam Protsenko (3): > >> fastboot: Fix slot names reported by getvar > > > > Commit [1] from this PR got replaced by series [2]. > > Replaced where ? [1] seems like a fix to me, [2] seems like feature > addition , Your instincts are correct (i.e. features must be rejected at this late stage) and they deserve credits. However and what seems extremely interesting is: - it is very easy to masquerade a fix as a feature (and viceversa), just by slightly adjusting the title [1-2] and it's very easy to play with your perception this way - it seems like there isn't a process (and this is a OSS-wide issue) of marking a patch as fix/feature (the latter would help the maintainer enormously) - it also appears that sometimes there is a fine line in our minds between what's a fix and what's a feature So, in a nutshell, series [2] is the v2 of commit [1]. IMHO there is no feature added. The two commits from series [2] are: - https://patchwork.ozlabs.org/patch/1116099/ - https://patchwork.ozlabs.org/patch/1116101/ What's interesting is that, depending on which version of fastboot utility users use/assume, they might see a "regression" introduced by [1-2]. Both [1-2] do changes to align U-Boot with latest fastboot utility from AOSP. What [2] is doing _in addition_ to [1] is: - remove some dead code in U-Boot which will never be exercised assuming users run the latest fastboot - place a bold warning in commit description that users are assumed to be using the latest fastboot utility > so merging [1] this late in the release cycle seems like the > right thing to do. My assumption is that Sam would like to see [1] being dropped in favor of [2], as commented in https://patchwork.ozlabs.org/patch/1114850/#2194140 > > > > [1] https://git.denx.de/?p=u-boot/u-boot-usb.git;a=commit;h=97a0c6ff577d57f162abc696c4efc962981229b1 > > [2] https://patchwork.ozlabs.org/cover/1116097/ ("fastboot: Support > > new A/B slot format") > >
On 6/15/19 4:24 PM, Eugeniu Rosca wrote: > Hi Marek, > > On Sat, Jun 15, 2019 at 02:35:12PM +0200, Marek Vasut wrote: >> On 6/15/19 11:46 AM, Eugeniu Rosca wrote: >>> Hello Marek, Lukasz cc: Sam >>> >>> On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: >>> [..] >>>> Sam Protsenko (3): >>>> fastboot: Fix slot names reported by getvar >>> >>> Commit [1] from this PR got replaced by series [2]. >> >> Replaced where ? [1] seems like a fix to me, [2] seems like feature >> addition , > > Your instincts are correct (i.e. features must be rejected at this late > stage) and they deserve credits. However and what seems extremely > interesting is: > - it is very easy to masquerade a fix as a feature (and viceversa), > just by slightly adjusting the title [1-2] and it's very easy to > play with your perception this way Likely because free software development is built on trust. If there are malicious actors who decide to not play nice, and even try to manipulate others by elaborately crafting commit messages or otherwise, this might work out to worm patches in. But at the end of the day, this helps no one, so please refrain from such underhanded practices. > - it seems like there isn't a process (and this is a OSS-wide issue) of > marking a patch as fix/feature (the latter would help the maintainer > enormously) Maybe you can start a separate thread and help figure out such a process? Complaining about unrelated stuff under random patches/PRs won't make that happen, it will only happen if someone takes the initiative. Scratching your own itch and all that. > - it also appears that sometimes there is a fine line in our minds > between what's a fix and what's a feature > > So, in a nutshell, series [2] is the v2 of commit [1]. > IMHO there is no feature added. > The two commits from series [2] are: > - https://patchwork.ozlabs.org/patch/1116099/ > - https://patchwork.ozlabs.org/patch/1116101/ Seems this was not clearly marked as so. I would suggest Sam sends a V3 on top of this PR and be done with it. > What's interesting is that, depending on which version of fastboot > utility users use/assume, they might see a "regression" introduced > by [1-2]. Both [1-2] do changes to align U-Boot with latest fastboot > utility from AOSP. What [2] is doing _in addition_ to [1] is: > - remove some dead code in U-Boot which will never be exercised > assuming users run the latest fastboot > - place a bold warning in commit description that users are assumed to > be using the latest fastboot utility If this can be fixed better, patches welcome. >> so merging [1] this late in the release cycle seems like the >> right thing to do. > > My assumption is that Sam would like to see [1] being dropped in favor > of [2], as commented in https://patchwork.ozlabs.org/patch/1114850/#2194140 I will leave that up to Sam and Lukasz to figure this out. However, I would prefer a V3 on top of this PR, that's easiest IMO. >>> [1] https://git.denx.de/?p=u-boot/u-boot-usb.git;a=commit;h=97a0c6ff577d57f162abc696c4efc962981229b1 >>> [2] https://patchwork.ozlabs.org/cover/1116097/ ("fastboot: Support >>> new A/B slot format") >>> >
On Sat, Jun 15, 2019 at 04:43:38PM +0200, Marek Vasut wrote: > On 6/15/19 4:24 PM, Eugeniu Rosca wrote: > > Hi Marek, > > > > On Sat, Jun 15, 2019 at 02:35:12PM +0200, Marek Vasut wrote: > >> On 6/15/19 11:46 AM, Eugeniu Rosca wrote: > >>> Hello Marek, Lukasz cc: Sam > >>> > >>> On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: > >>> [..] > >>>> Sam Protsenko (3): > >>>> fastboot: Fix slot names reported by getvar > >>> > >>> Commit [1] from this PR got replaced by series [2]. > >> > >> Replaced where ? [1] seems like a fix to me, [2] seems like feature > >> addition , > > > > Your instincts are correct (i.e. features must be rejected at this late > > stage) and they deserve credits. However and what seems extremely > > interesting is: > > - it is very easy to masquerade a fix as a feature (and viceversa), > > just by slightly adjusting the title [1-2] and it's very easy to > > play with your perception this way > > Likely because free software development is built on trust. If there are > malicious actors who decide to not play nice, and even try to manipulate > others by elaborately crafting commit messages or otherwise, this might > work out to worm patches in. But at the end of the day, this helps no > one, so please refrain from such underhanded practices. Yes, as a community we expect people to be clear and, well, plainly spoken, perhaps is the best way to say it. A bug fix is a bug fix and a new feature is a new feature. > > - it seems like there isn't a process (and this is a OSS-wide issue) of > > marking a patch as fix/feature (the latter would help the maintainer > > enormously) > > Maybe you can start a separate thread and help figure out such a > process? Complaining about unrelated stuff under random patches/PRs > won't make that happen, it will only happen if someone takes the > initiative. Scratching your own itch and all that. > > > - it also appears that sometimes there is a fine line in our minds > > between what's a fix and what's a feature > > > > So, in a nutshell, series [2] is the v2 of commit [1]. > > IMHO there is no feature added. > > The two commits from series [2] are: > > - https://patchwork.ozlabs.org/patch/1116099/ > > - https://patchwork.ozlabs.org/patch/1116101/ > > Seems this was not clearly marked as so. > I would suggest Sam sends a V3 on top of this PR and be done with it. Agreed. > > What's interesting is that, depending on which version of fastboot > > utility users use/assume, they might see a "regression" introduced > > by [1-2]. Both [1-2] do changes to align U-Boot with latest fastboot > > utility from AOSP. What [2] is doing _in addition_ to [1] is: > > - remove some dead code in U-Boot which will never be exercised > > assuming users run the latest fastboot > > - place a bold warning in commit description that users are assumed to > > be using the latest fastboot utility > > If this can be fixed better, patches welcome. > > >> so merging [1] this late in the release cycle seems like the > >> right thing to do. > > > > My assumption is that Sam would like to see [1] being dropped in favor > > of [2], as commented in https://patchwork.ozlabs.org/patch/1114850/#2194140 > > I will leave that up to Sam and Lukasz to figure this out. > However, I would prefer a V3 on top of this PR, that's easiest IMO. Agreed. Thanks all!
Hi Marek, On Sat, Jun 15, 2019 at 6:16 PM Marek Vasut <marex@denx.de> wrote: > > On 6/15/19 4:24 PM, Eugeniu Rosca wrote: > > Hi Marek, > > > > On Sat, Jun 15, 2019 at 02:35:12PM +0200, Marek Vasut wrote: > >> On 6/15/19 11:46 AM, Eugeniu Rosca wrote: > >>> Hello Marek, Lukasz cc: Sam > >>> > >>> On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: > >>> [..] > >>>> Sam Protsenko (3): > >>>> fastboot: Fix slot names reported by getvar > >>> > >>> Commit [1] from this PR got replaced by series [2]. > >> > >> Replaced where ? [1] seems like a fix to me, [2] seems like feature > >> addition , > > > > Your instincts are correct (i.e. features must be rejected at this late > > stage) and they deserve credits. However and what seems extremely > > interesting is: > > - it is very easy to masquerade a fix as a feature (and viceversa), > > just by slightly adjusting the title [1-2] and it's very easy to > > play with your perception this way > > Likely because free software development is built on trust. If there are > malicious actors who decide to not play nice, and even try to manipulate > others by elaborately crafting commit messages or otherwise, this might > work out to worm patches in. But at the end of the day, this helps no > one, so please refrain from such underhanded practices. > I believe it's for maintainer to decide, which patches are ok to go to -rc, and which are not. We can always discuss that here in mailing list, if something is not clear. > > - it seems like there isn't a process (and this is a OSS-wide issue) of > > marking a patch as fix/feature (the latter would help the maintainer > > enormously) > > Maybe you can start a separate thread and help figure out such a > process? Complaining about unrelated stuff under random patches/PRs > won't make that happen, it will only happen if someone takes the > initiative. Scratching your own itch and all that. > > > - it also appears that sometimes there is a fine line in our minds > > between what's a fix and what's a feature > > > > So, in a nutshell, series [2] is the v2 of commit [1]. > > IMHO there is no feature added. > > The two commits from series [2] are: > > - https://patchwork.ozlabs.org/patch/1116099/ > > - https://patchwork.ozlabs.org/patch/1116101/ > > Seems this was not clearly marked as so. > I would suggest Sam sends a V3 on top of this PR and be done with it. > Agreed. From my point of view, both V2 and V3 are simultaneously harmless *and* not required in this release. Just because there are no users for A/B slotted partitions in upstream U-Boot right now. The only important fix to be included in release, is this patch-series: [PATCH v3 0/3] fastboot: Fix getvar "has-slot" and cleanup which actually fixes fastboot on non-slotted partitions (like on BeagleBoard X15), where right now we are unable to flash images. > > What's interesting is that, depending on which version of fastboot > > utility users use/assume, they might see a "regression" introduced > > by [1-2]. Both [1-2] do changes to align U-Boot with latest fastboot > > utility from AOSP. What [2] is doing _in addition_ to [1] is: > > - remove some dead code in U-Boot which will never be exercised > > assuming users run the latest fastboot > > - place a bold warning in commit description that users are assumed to > > be using the latest fastboot utility > > If this can be fixed better, patches welcome. > > >> so merging [1] this late in the release cycle seems like the > >> right thing to do. > > > > My assumption is that Sam would like to see [1] being dropped in favor > > of [2], as commented in https://patchwork.ozlabs.org/patch/1114850/#2194140 > > I will leave that up to Sam and Lukasz to figure this out. > However, I would prefer a V3 on top of this PR, that's easiest IMO. > Let's make it so. I can send V3 later. Discussed patch ("fastboot: Fix slot names reported by getvar") is not crucial anyway, I guess. Thanks! > >>> [1] https://git.denx.de/?p=u-boot/u-boot-usb.git;a=commit;h=97a0c6ff577d57f162abc696c4efc962981229b1 > >>> [2] https://patchwork.ozlabs.org/cover/1116097/ ("fastboot: Support > >>> new A/B slot format") > >>> > > > > > -- > Best regards, > Marek Vasut
On 6/18/19 1:42 PM, Sam Protsenko wrote: > Hi Marek, > > On Sat, Jun 15, 2019 at 6:16 PM Marek Vasut <marex@denx.de> wrote: >> >> On 6/15/19 4:24 PM, Eugeniu Rosca wrote: >>> Hi Marek, >>> >>> On Sat, Jun 15, 2019 at 02:35:12PM +0200, Marek Vasut wrote: >>>> On 6/15/19 11:46 AM, Eugeniu Rosca wrote: >>>>> Hello Marek, Lukasz cc: Sam >>>>> >>>>> On Sat, Jun 15, 2019 at 5:23 AM Marek Vasut <marex@denx.de> wrote: >>>>> [..] >>>>>> Sam Protsenko (3): >>>>>> fastboot: Fix slot names reported by getvar >>>>> >>>>> Commit [1] from this PR got replaced by series [2]. >>>> >>>> Replaced where ? [1] seems like a fix to me, [2] seems like feature >>>> addition , >>> >>> Your instincts are correct (i.e. features must be rejected at this late >>> stage) and they deserve credits. However and what seems extremely >>> interesting is: >>> - it is very easy to masquerade a fix as a feature (and viceversa), >>> just by slightly adjusting the title [1-2] and it's very easy to >>> play with your perception this way >> >> Likely because free software development is built on trust. If there are >> malicious actors who decide to not play nice, and even try to manipulate >> others by elaborately crafting commit messages or otherwise, this might >> work out to worm patches in. But at the end of the day, this helps no >> one, so please refrain from such underhanded practices. >> > > I believe it's for maintainer to decide, which patches are ok to go to > -rc, and which are not. We can always discuss that here in mailing > list, if something is not clear. > >>> - it seems like there isn't a process (and this is a OSS-wide issue) of >>> marking a patch as fix/feature (the latter would help the maintainer >>> enormously) >> >> Maybe you can start a separate thread and help figure out such a >> process? Complaining about unrelated stuff under random patches/PRs >> won't make that happen, it will only happen if someone takes the >> initiative. Scratching your own itch and all that. >> >>> - it also appears that sometimes there is a fine line in our minds >>> between what's a fix and what's a feature >>> >>> So, in a nutshell, series [2] is the v2 of commit [1]. >>> IMHO there is no feature added. >>> The two commits from series [2] are: >>> - https://patchwork.ozlabs.org/patch/1116099/ >>> - https://patchwork.ozlabs.org/patch/1116101/ >> >> Seems this was not clearly marked as so. >> I would suggest Sam sends a V3 on top of this PR and be done with it. >> > > Agreed. From my point of view, both V2 and V3 are simultaneously > harmless *and* not required in this release. Just because there are no > users for A/B slotted partitions in upstream U-Boot right now. > > The only important fix to be included in release, is this patch-series: > > [PATCH v3 0/3] fastboot: Fix getvar "has-slot" and cleanup > > which actually fixes fastboot on non-slotted partitions (like on > BeagleBoard X15), where right now we are unable to flash images. > >>> What's interesting is that, depending on which version of fastboot >>> utility users use/assume, they might see a "regression" introduced >>> by [1-2]. Both [1-2] do changes to align U-Boot with latest fastboot >>> utility from AOSP. What [2] is doing _in addition_ to [1] is: >>> - remove some dead code in U-Boot which will never be exercised >>> assuming users run the latest fastboot >>> - place a bold warning in commit description that users are assumed to >>> be using the latest fastboot utility >> >> If this can be fixed better, patches welcome. >> >>>> so merging [1] this late in the release cycle seems like the >>>> right thing to do. >>> >>> My assumption is that Sam would like to see [1] being dropped in favor >>> of [2], as commented in https://patchwork.ozlabs.org/patch/1114850/#2194140 >> >> I will leave that up to Sam and Lukasz to figure this out. >> However, I would prefer a V3 on top of this PR, that's easiest IMO. >> > > Let's make it so. I can send V3 later. Discussed patch ("fastboot: Fix > slot names reported by getvar") is not crucial anyway, I guess. Please do, the USB PR is now in.
On Fri, Jun 14, 2019 at 12:45:47PM +0200, Marek Vasut wrote: > Assorted gadget fixes. > > The following changes since commit c2ea87883ef309570c8903e6de4b8b78685d73d0: > > Merge tag 'efi-2019-07-rc5' of git://git.denx.de/u-boot-efi > (2019-06-12 07:15:38 -0400) > > are available in the Git repository at: > > git://git.denx.de/u-boot-usb.git master > > for you to fetch changes up to 220f655176de8e6aa4aaea91bb2182260d26c4a4: > > fastboot: Check if partition really exist in getvar_has_slot() > (2019-06-14 12:39:54 +0200) > Applied to u-boot/master, thanks!
Assorted gadget fixes. The following changes since commit c2ea87883ef309570c8903e6de4b8b78685d73d0: Merge tag 'efi-2019-07-rc5' of git://git.denx.de/u-boot-efi (2019-06-12 07:15:38 -0400) are available in the Git repository at: git://git.denx.de/u-boot-usb.git master for you to fetch changes up to 220f655176de8e6aa4aaea91bb2182260d26c4a4: fastboot: Check if partition really exist in getvar_has_slot() (2019-06-14 12:39:54 +0200) ---------------------------------------------------------------- Igor Opaniuk (1): fastboot: Check if partition really exist in getvar_has_slot() Marek Vasut (1): spl: dfu: Fix printed variable name Sam Protsenko (3): fastboot: Fix slot names reported by getvar fastboot: Use const qualifier for char *part_name fastboot: getvar: Refactor fastboot_*_get_part_info() usage Sjoerd Simons (1): usb: gadget: error out if g_dnl registration fails cmd/usb_gadget_sdp.c | 11 ++++++++--- common/spl/spl_dfu.c | 2 +- common/spl/spl_sdp.c | 6 +++++- drivers/fastboot/fb_getvar.c | 103 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++------------------------ drivers/fastboot/fb_mmc.c | 3 ++- drivers/fastboot/fb_nand.c | 4 ++-- include/fb_mmc.h | 3 ++- include/fb_nand.h | 4 ++-- 8 files changed, 101 insertions(+), 35 deletions(-)