| Message ID | 20210818221920.3893-1-digetx@gmail.com |
|---|---|
| Headers | show |
| Series | Support EFI partition on NVIDIA Tegra devices | expand |
Looks good,
Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, Aug 19, 2021 at 01:19:17AM +0300, Dmitry Osipenko wrote: > Support looking up GPT at a non-standard location specified by a block > device driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Looks good, Reviewed-by: Christoph Hellwig <hch@lst.de>
On Thu, 19 Aug 2021, Dmitry Osipenko wrote: >Support looking up GPT at a non-standard location specified by a block >device driver. > >Signed-off-by: Dmitry Osipenko <digetx@gmail.com> Acked-by: Davidlohr Bueso <dbueso@suse.de>
On Thu, 19 Aug 2021, Dmitry Osipenko wrote: > - Moved GPT calculation into MMC core and added MMC_CAP2_ALT_GPT_SECTOR > flag, like it was asked by Ulf Hansson. Me and Thierry have concerns > about whether it's better to have Tegra-specific function in a core > instead of Tegra driver, but it also works, so I decided to try that > variant. I think this is better as you had it in v5. This is specific to tegra and shouldn't be in generic code. Thanks, Davidlohr
19.08.2021 20:18, Davidlohr Bueso пишет: > On Thu, 19 Aug 2021, Dmitry Osipenko wrote: > >> - Moved GPT calculation into MMC core and added >> MMC_CAP2_ALT_GPT_SECTOR >> flag, like it was asked by Ulf Hansson. Me and Thierry have concerns >> about whether it's better to have Tegra-specific function in a core >> instead of Tegra driver, but it also works, so I decided to try that >> variant. > > I think this is better as you had it in v5. This is specific to tegra and > shouldn't be in generic code. Yeah, but Ulf wants it to be in core. On the other hand, MMC core already carries all kinds of quirks for hosts and cards, so it's not something extraordinary for the MMC.
On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: > This series adds the most minimal EFI partition support for NVIDIA Tegra > consumer devices, like Android tablets and game consoles, making theirs > eMMC accessible out-of-the-box using downstream bootloader and mainline > Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console > that are already well supported in mainline and internal storage is the > only biggest thing left to support. [...] Could we provide the GPT sector via DT? As I understand this is for non-removable eMMC storage. It would remove the need for a cap bit and hardcoded calculations instead just checking if DT node of the controller contains a magic entry with a number. Best Regards Michał Mirosław
On Thu, Aug 19, 2021 at 01:19:17AM +0300, Dmitry Osipenko wrote: > Support looking up GPT at a non-standard location specified by a block > device driver. > > Signed-off-by: Dmitry Osipenko <digetx@gmail.com> > --- > block/partitions/efi.c | 12 ++++++++++++ > 1 file changed, 12 insertions(+) > > diff --git a/block/partitions/efi.c b/block/partitions/efi.c > index aaa3dc487cb5..7ca5c4c374d4 100644 > --- a/block/partitions/efi.c > +++ b/block/partitions/efi.c > @@ -585,6 +585,8 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt, > gpt_header *pgpt = NULL, *agpt = NULL; > gpt_entry *pptes = NULL, *aptes = NULL; > legacy_mbr *legacymbr; > + struct gendisk *disk = state->disk; > + const struct block_device_operations *fops = disk->fops; > sector_t total_sectors = get_capacity(state->disk); > u64 lastlba; > > @@ -619,6 +621,16 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt, > if (!good_agpt && force_gpt) > good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes); > > + if (!good_agpt && force_gpt && fops->alternative_gpt_sector) { > + sector_t agpt_sector; > + int err; > + > + err = fops->alternative_gpt_sector(disk, &agpt_sector); > + if (!err) > + good_agpt = is_gpt_valid(state, agpt_sector, > + &agpt, &aptes); > + } > + When alternative_gpt_sector is provided I would expect it to override the default, not be a fallback for it. But if someone tries to put a broken (decoy, garbage) GPT at a standard place, current ordering will prevent overriding it. Best Regards Michał Mirosław
21.08.2021 01:45, Michał Mirosław пишет: > On Thu, Aug 19, 2021 at 01:19:17AM +0300, Dmitry Osipenko wrote: >> Support looking up GPT at a non-standard location specified by a block >> device driver. >> >> Signed-off-by: Dmitry Osipenko <digetx@gmail.com> >> --- >> block/partitions/efi.c | 12 ++++++++++++ >> 1 file changed, 12 insertions(+) >> >> diff --git a/block/partitions/efi.c b/block/partitions/efi.c >> index aaa3dc487cb5..7ca5c4c374d4 100644 >> --- a/block/partitions/efi.c >> +++ b/block/partitions/efi.c >> @@ -585,6 +585,8 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt, >> gpt_header *pgpt = NULL, *agpt = NULL; >> gpt_entry *pptes = NULL, *aptes = NULL; >> legacy_mbr *legacymbr; >> + struct gendisk *disk = state->disk; >> + const struct block_device_operations *fops = disk->fops; >> sector_t total_sectors = get_capacity(state->disk); >> u64 lastlba; >> >> @@ -619,6 +621,16 @@ static int find_valid_gpt(struct parsed_partitions *state, gpt_header **gpt, >> if (!good_agpt && force_gpt) >> good_agpt = is_gpt_valid(state, lastlba, &agpt, &aptes); >> >> + if (!good_agpt && force_gpt && fops->alternative_gpt_sector) { >> + sector_t agpt_sector; >> + int err; >> + >> + err = fops->alternative_gpt_sector(disk, &agpt_sector); >> + if (!err) >> + good_agpt = is_gpt_valid(state, agpt_sector, >> + &agpt, &aptes); >> + } >> + > > When alternative_gpt_sector is provided I would expect it to override > the default, not be a fallback for it. But if someone tries to put > a broken (decoy, garbage) GPT at a standard place, current ordering > will prevent overriding it. This will break devices that have GPT at standard location. If someone tries to manipulate with GPT entries, then it's a problem of that someone.
21.08.2021 01:41, Michał Mirosław пишет: > On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: >> This series adds the most minimal EFI partition support for NVIDIA Tegra >> consumer devices, like Android tablets and game consoles, making theirs >> eMMC accessible out-of-the-box using downstream bootloader and mainline >> Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console >> that are already well supported in mainline and internal storage is the >> only biggest thing left to support. > [...] > > Could we provide the GPT sector via DT? As I understand this is for > non-removable eMMC storage. It would remove the need for a cap bit and > hardcoded calculations instead just checking if DT node of the controller > contains a magic entry with a number. The same device model usually comes in different flavors that have a different eMMC unit and size. So no, it can't be hardcoded in DT.
On Sat, Aug 21, 2021 at 08:27:15PM +0300, Dmitry Osipenko wrote: > 21.08.2021 01:41, Michał Mirosław пишет: > > On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: > >> This series adds the most minimal EFI partition support for NVIDIA Tegra > >> consumer devices, like Android tablets and game consoles, making theirs > >> eMMC accessible out-of-the-box using downstream bootloader and mainline > >> Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console > >> that are already well supported in mainline and internal storage is the > >> only biggest thing left to support. > > [...] > > > > Could we provide the GPT sector via DT? As I understand this is for > > non-removable eMMC storage. It would remove the need for a cap bit and > > hardcoded calculations instead just checking if DT node of the controller > > contains a magic entry with a number. > > The same device model usually comes in different flavors that have a > different eMMC unit and size. So no, it can't be hardcoded in DT. I see. I was thinking how to avoid of going the whole way and creating another controller capability (since this is going to be core code) - could this workaround be enabled just by a boolean DT property at controller's node instead? Or do we expect non-DT platforms to be similarly broken? Best Regards Michał Mirosław
On Tue, Aug 24, 2021 at 01:40:02AM +0200, Michał Mirosław wrote: > On Sat, Aug 21, 2021 at 08:27:15PM +0300, Dmitry Osipenko wrote: > > 21.08.2021 01:41, Michał Mirosław пишет: > > > On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: > > >> This series adds the most minimal EFI partition support for NVIDIA Tegra > > >> consumer devices, like Android tablets and game consoles, making theirs > > >> eMMC accessible out-of-the-box using downstream bootloader and mainline > > >> Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console > > >> that are already well supported in mainline and internal storage is the > > >> only biggest thing left to support. > > > [...] > > > > > > Could we provide the GPT sector via DT? As I understand this is for > > > non-removable eMMC storage. It would remove the need for a cap bit and > > > hardcoded calculations instead just checking if DT node of the controller > > > contains a magic entry with a number. > > > > The same device model usually comes in different flavors that have a > > different eMMC unit and size. So no, it can't be hardcoded in DT. > > I see. I was thinking how to avoid of going the whole way and creating > another controller capability (since this is going to be core code) - > could this workaround be enabled just by a boolean DT property at > controller's node instead? Or do we expect non-DT platforms to be > similarly broken? Rewording my concern: I believe that this is platform's and not a controller's misfeature, so the controller driver feels like wrong place fix. That's why I'd prefer that the enable came from the DT and not from driver's code. Best Regards Michał Mirosław
24.08.2021 13:38, Michał Mirosław пишет: > On Tue, Aug 24, 2021 at 01:40:02AM +0200, Michał Mirosław wrote: >> On Sat, Aug 21, 2021 at 08:27:15PM +0300, Dmitry Osipenko wrote: >>> 21.08.2021 01:41, Michał Mirosław пишет: >>>> On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: >>>>> This series adds the most minimal EFI partition support for NVIDIA Tegra >>>>> consumer devices, like Android tablets and game consoles, making theirs >>>>> eMMC accessible out-of-the-box using downstream bootloader and mainline >>>>> Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console >>>>> that are already well supported in mainline and internal storage is the >>>>> only biggest thing left to support. >>>> [...] >>>> >>>> Could we provide the GPT sector via DT? As I understand this is for >>>> non-removable eMMC storage. It would remove the need for a cap bit and >>>> hardcoded calculations instead just checking if DT node of the controller >>>> contains a magic entry with a number. >>> >>> The same device model usually comes in different flavors that have a >>> different eMMC unit and size. So no, it can't be hardcoded in DT. >> >> I see. I was thinking how to avoid of going the whole way and creating >> another controller capability (since this is going to be core code) - >> could this workaround be enabled just by a boolean DT property at >> controller's node instead? Or do we expect non-DT platforms to be >> similarly broken? > > Rewording my concern: I believe that this is platform's and not > a controller's misfeature, so the controller driver feels like wrong > place fix. That's why I'd prefer that the enable came from the DT > and not from driver's code. The alternative GPT entry requires user to add 'gpt' argument to kernel's cmdline. If board already uses proper alternative GPT entry at the last sector, then nothing changed for that board. The case where board uses 'gpt' cmdline + it had stale GPT entry at the special location used by Android devices and chance that now suddenly that GPT entry will pop up is close to zero. All old partition table entries should be erased on reparation. If it wasn't done, then it's not a kernel's problem, it's much more a user's problem. Even though kernel could help that poor user if will be really needed. There is no reason to over-engineer unless somebody will tell that it broke the very special board. Neither of currently supported boards should require more quirks. Hence, why bother?
On Tue, Aug 24, 2021 at 07:06:18PM +0300, Dmitry Osipenko wrote: > 24.08.2021 13:38, Michał Mirosław пишет: > > On Tue, Aug 24, 2021 at 01:40:02AM +0200, Michał Mirosław wrote: > >> On Sat, Aug 21, 2021 at 08:27:15PM +0300, Dmitry Osipenko wrote: > >>> 21.08.2021 01:41, Michał Mirosław пишет: > >>>> On Thu, Aug 19, 2021 at 01:19:15AM +0300, Dmitry Osipenko wrote: > >>>>> This series adds the most minimal EFI partition support for NVIDIA Tegra > >>>>> consumer devices, like Android tablets and game consoles, making theirs > >>>>> eMMC accessible out-of-the-box using downstream bootloader and mainline > >>>>> Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console > >>>>> that are already well supported in mainline and internal storage is the > >>>>> only biggest thing left to support. > >>>> [...] > >>>> > >>>> Could we provide the GPT sector via DT? As I understand this is for > >>>> non-removable eMMC storage. It would remove the need for a cap bit and > >>>> hardcoded calculations instead just checking if DT node of the controller > >>>> contains a magic entry with a number. > >>> > >>> The same device model usually comes in different flavors that have a > >>> different eMMC unit and size. So no, it can't be hardcoded in DT. > >> > >> I see. I was thinking how to avoid of going the whole way and creating > >> another controller capability (since this is going to be core code) - > >> could this workaround be enabled just by a boolean DT property at > >> controller's node instead? Or do we expect non-DT platforms to be > >> similarly broken? > > > > Rewording my concern: I believe that this is platform's and not > > a controller's misfeature, so the controller driver feels like wrong > > place fix. That's why I'd prefer that the enable came from the DT > > and not from driver's code. > > The alternative GPT entry requires user to add 'gpt' argument to > kernel's cmdline. If board already uses proper alternative GPT entry at > the last sector, then nothing changed for that board. > > The case where board uses 'gpt' cmdline + it had stale GPT entry at the > special location used by Android devices and chance that now suddenly > that GPT entry will pop up is close to zero. > > All old partition table entries should be erased on reparation. If it > wasn't done, then it's not a kernel's problem, it's much more a user's > problem. Even though kernel could help that poor user if will be really > needed. > > There is no reason to over-engineer unless somebody will tell that it > broke the very special board. Neither of currently supported boards > should require more quirks. Hence, why bother? You could drop patch 4 from v7 if you checked DT boolean property instead of adding a capability in patch 3. (Patch 4 would be replaced by DT changes for relevant boards.) Best Regards Michał Mirosław
This series adds the most minimal EFI partition support for NVIDIA Tegra consumer devices, like Android tablets and game consoles, making theirs eMMC accessible out-of-the-box using downstream bootloader and mainline Linux kernel. eMMC now works on Acer A500 tablet and Ouya game console that are already well supported in mainline and internal storage is the only biggest thing left to support. Changelog: v6: - Added comment for the alternative_gpt_sector() callback, which was asked by Christoph Hellwig. - Changed alternative_gpt_sector() to take disk for the argument instead of blkdev. This was asked by Christoph Hellwig. - Dropped mmc_bdops check as it was suggested by Christoph Hellwig. - Added missing mmc_blk_put() that was spotted by Christoph Hellwig. - Moved GPT calculation into MMC core and added MMC_CAP2_ALT_GPT_SECTOR flag, like it was asked by Ulf Hansson. Me and Thierry have concerns about whether it's better to have Tegra-specific function in a core instead of Tegra driver, but it also works, so I decided to try that variant. v5: - Implemented alternative_gpt_sector() blk/mmc callback that was suggested by Christoph Hellwig in a comment to v4. - mmc_bdev_to_card() now checks blk fops instead of the major number, like it was suggested by Christoph Hellwig in a comment to v4. - Emailed Rob Herring, which was asked by Ulf Hansson in a comment to v4. Although the of-match change is gone now in v5, the matching is transformed into the new SDHCI quirk of the Tegra driver. v4: - Rebased on top of recent linux-next. v3: - Removed unnecessary v1 hunk that was left by accident in efi.c of v2. v2: - This is continuation of [1] where Davidlohr Bueso suggested that it should be better to avoid supporting in mainline the custom gpt_sector kernel cmdline parameter that downstream Android kernels use. We can do this for the devices that are already mainlined, so I dropped the cmdline from the v2 and left only the variant with a fixed GPT address. [1] https://lore.kernel.org/linux-efi/20210327212100.3834-3-digetx@gmail.com/T/ Dmitry Osipenko (5): block: Add alternative_gpt_sector() operation partitions/efi: Support non-standard GPT location mmc: core: Add raw_boot_mult field to mmc_ext_csd mmc: block: Support alternative_gpt_sector() operation mmc: sdhci-tegra: Enable MMC_CAP2_ALT_GPT_SECTOR block/partitions/efi.c | 12 ++++++++++++ drivers/mmc/core/block.c | 21 ++++++++++++++++++++ drivers/mmc/core/core.c | 35 ++++++++++++++++++++++++++++++++++ drivers/mmc/core/core.h | 2 ++ drivers/mmc/core/mmc.c | 2 ++ drivers/mmc/host/sdhci-tegra.c | 9 +++++++++ include/linux/blkdev.h | 7 +++++++ include/linux/mmc/card.h | 1 + include/linux/mmc/host.h | 1 + 9 files changed, 90 insertions(+)