| Message ID | 1312228986-32307-8-git-send-email-a.heider@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
On 08/01/2011 01:02 PM, Andre Heider wrote: > The driver implements a character and misc device, meant for the > axed OtherOS to exchange various settings with GameOS. > Since Firmware 3.21 there is no GameOS support anymore to write these > settings, so limit the driver to the OtherOS environment. This is really a test if running on the PS3 OtherOS, so this comment should state that. > > Signed-off-by: Andre Heider <a.heider@gmail.com> > --- > arch/powerpc/platforms/ps3/Kconfig | 1 + > drivers/char/ps3flash.c | 7 +++++++ > 2 files changed, 8 insertions(+), 0 deletions(-) > > diff --git a/arch/powerpc/platforms/ps3/Kconfig b/arch/powerpc/platforms/ps3/Kconfig > index 84df5c8..5eb956a 100644 > --- a/arch/powerpc/platforms/ps3/Kconfig > +++ b/arch/powerpc/platforms/ps3/Kconfig > @@ -121,6 +121,7 @@ config PS3_FLASH > > This support is required to access the PS3 FLASH ROM, which > contains the boot loader and some boot options. > + This driver only supports the deprecated OtherOS LPAR. This will be confusing for OtherOS users, so should be removed. > In general, all users will say Y or M. This could be changed to: 'In general, all PS3 OtherOS users will say Y or M.' > > As this driver needs a fixed buffer of 256 KiB of memory, it can > diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c > index 69c734a..b1e8659 100644 > --- a/drivers/char/ps3flash.c > +++ b/drivers/char/ps3flash.c > @@ -25,6 +25,7 @@ > > #include <asm/lv1call.h> > #include <asm/ps3stor.h> > +#include <asm/firmware.h> > > > #define DEVICE_NAME "ps3flash" > @@ -455,6 +456,12 @@ static struct ps3_system_bus_driver ps3flash = { > > static int __init ps3flash_init(void) > { > + if (!firmware_has_feature(FW_FEATURE_PS3_LV1)) > + return -ENODEV; Is this needed? Won't this driver only be loaded on PS3 hardware? > + > + if (ps3_get_ss_laid() != PS3_SS_LAID_OTHEROS) > + return -ENODEV; > + > return ps3_system_bus_driver_register(&ps3flash); > } > -Geoff
On Thu, Aug 4, 2011 at 12:34 AM, Geoff Levand <geoff@infradead.org> wrote: > On 08/01/2011 01:02 PM, Andre Heider wrote: >> The driver implements a character and misc device, meant for the >> axed OtherOS to exchange various settings with GameOS. >> Since Firmware 3.21 there is no GameOS support anymore to write these >> settings, so limit the driver to the OtherOS environment. > > This is really a test if running on the PS3 OtherOS, so this > comment should state that. Ok. > >> >> Signed-off-by: Andre Heider <a.heider@gmail.com> >> --- >> arch/powerpc/platforms/ps3/Kconfig | 1 + >> drivers/char/ps3flash.c | 7 +++++++ >> 2 files changed, 8 insertions(+), 0 deletions(-) >> >> diff --git a/arch/powerpc/platforms/ps3/Kconfig b/arch/powerpc/platforms/ps3/Kconfig >> index 84df5c8..5eb956a 100644 >> --- a/arch/powerpc/platforms/ps3/Kconfig >> +++ b/arch/powerpc/platforms/ps3/Kconfig >> @@ -121,6 +121,7 @@ config PS3_FLASH >> >> This support is required to access the PS3 FLASH ROM, which >> contains the boot loader and some boot options. >> + This driver only supports the deprecated OtherOS LPAR. > > This will be confusing for OtherOS users, so should be removed. Ok. > >> In general, all users will say Y or M. > > > This could be changed to: 'In general, all PS3 OtherOS users will say Y or M.' Yeah, you're right, that's much better > >> >> As this driver needs a fixed buffer of 256 KiB of memory, it can >> diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c >> index 69c734a..b1e8659 100644 >> --- a/drivers/char/ps3flash.c >> +++ b/drivers/char/ps3flash.c >> @@ -25,6 +25,7 @@ >> >> #include <asm/lv1call.h> >> #include <asm/ps3stor.h> >> +#include <asm/firmware.h> >> >> >> #define DEVICE_NAME "ps3flash" >> @@ -455,6 +456,12 @@ static struct ps3_system_bus_driver ps3flash = { >> >> static int __init ps3flash_init(void) >> { >> + if (!firmware_has_feature(FW_FEATURE_PS3_LV1)) >> + return -ENODEV; > > Is this needed? Won't this driver only be loaded on PS3 hardware? > The same code is in drivers/block/ps3disk.c, I wasn't sure if it is missing here or redundant there. Should I remove it here? >> + >> + if (ps3_get_ss_laid() != PS3_SS_LAID_OTHEROS) >> + return -ENODEV; >> + >> return ps3_system_bus_driver_register(&ps3flash); >> } >> > > -Geoff > >
On Thu, Aug 4, 2011 at 18:40, Andre Heider <a.heider@gmail.com> wrote: > On Thu, Aug 4, 2011 at 12:34 AM, Geoff Levand <geoff@infradead.org> wrote: >> On 08/01/2011 01:02 PM, Andre Heider wrote: >>> --- a/drivers/char/ps3flash.c >>> +++ b/drivers/char/ps3flash.c >>> @@ -25,6 +25,7 @@ >>> >>> #include <asm/lv1call.h> >>> #include <asm/ps3stor.h> >>> +#include <asm/firmware.h> >>> >>> >>> #define DEVICE_NAME "ps3flash" >>> @@ -455,6 +456,12 @@ static struct ps3_system_bus_driver ps3flash = { >>> >>> static int __init ps3flash_init(void) >>> { >>> + if (!firmware_has_feature(FW_FEATURE_PS3_LV1)) >>> + return -ENODEV; >> >> Is this needed? Won't this driver only be loaded on PS3 hardware? >> > > The same code is in drivers/block/ps3disk.c, I wasn't sure if it is > missing here or redundant there. > Should I remove it here? > >>> + >>> + if (ps3_get_ss_laid() != PS3_SS_LAID_OTHEROS) >>> + return -ENODEV; >>> + >>> return ps3_system_bus_driver_register(&ps3flash); >>> } ps3flash_init() is called straight from module_init(), so it could be called on non-PS3. ps3_system_bus_driver_register() has the firmware_has_feature_check(), so it will reject non-PS3. But if your *_init() does any processing before calling ps3_system_bus_driver_register() (like ps3disk_init() does, and ps3flash_init() now does due to your patch), you have to do the check yourself, to make sure it returns early on non-PS3. Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Thu, Aug 4, 2011 at 9:27 PM, Geert Uytterhoeven <geert@linux-m68k.org> wrote: > On Thu, Aug 4, 2011 at 18:40, Andre Heider <a.heider@gmail.com> wrote: >> On Thu, Aug 4, 2011 at 12:34 AM, Geoff Levand <geoff@infradead.org> wrote: >>> On 08/01/2011 01:02 PM, Andre Heider wrote: >>>> --- a/drivers/char/ps3flash.c >>>> +++ b/drivers/char/ps3flash.c >>>> @@ -25,6 +25,7 @@ >>>> >>>> #include <asm/lv1call.h> >>>> #include <asm/ps3stor.h> >>>> +#include <asm/firmware.h> >>>> >>>> >>>> #define DEVICE_NAME "ps3flash" >>>> @@ -455,6 +456,12 @@ static struct ps3_system_bus_driver ps3flash = { >>>> >>>> static int __init ps3flash_init(void) >>>> { >>>> + if (!firmware_has_feature(FW_FEATURE_PS3_LV1)) >>>> + return -ENODEV; >>> >>> Is this needed? Won't this driver only be loaded on PS3 hardware? >>> >> >> The same code is in drivers/block/ps3disk.c, I wasn't sure if it is >> missing here or redundant there. >> Should I remove it here? >> >>>> + >>>> + if (ps3_get_ss_laid() != PS3_SS_LAID_OTHEROS) >>>> + return -ENODEV; >>>> + >>>> return ps3_system_bus_driver_register(&ps3flash); >>>> } > > ps3flash_init() is called straight from module_init(), so it could be > called on non-PS3. > ps3_system_bus_driver_register() has the firmware_has_feature_check(), > so it will > reject non-PS3. > > But if your *_init() does any processing before calling > ps3_system_bus_driver_register() > (like ps3disk_init() does, and ps3flash_init() now does due to your > patch), you have to > do the check yourself, to make sure it returns early on non-PS3. Aha, makes perfect sense. Thanks Geert
diff --git a/arch/powerpc/platforms/ps3/Kconfig b/arch/powerpc/platforms/ps3/Kconfig index 84df5c8..5eb956a 100644 --- a/arch/powerpc/platforms/ps3/Kconfig +++ b/arch/powerpc/platforms/ps3/Kconfig @@ -121,6 +121,7 @@ config PS3_FLASH This support is required to access the PS3 FLASH ROM, which contains the boot loader and some boot options. + This driver only supports the deprecated OtherOS LPAR. In general, all users will say Y or M. As this driver needs a fixed buffer of 256 KiB of memory, it can diff --git a/drivers/char/ps3flash.c b/drivers/char/ps3flash.c index 69c734a..b1e8659 100644 --- a/drivers/char/ps3flash.c +++ b/drivers/char/ps3flash.c @@ -25,6 +25,7 @@ #include <asm/lv1call.h> #include <asm/ps3stor.h> +#include <asm/firmware.h> #define DEVICE_NAME "ps3flash" @@ -455,6 +456,12 @@ static struct ps3_system_bus_driver ps3flash = { static int __init ps3flash_init(void) { + if (!firmware_has_feature(FW_FEATURE_PS3_LV1)) + return -ENODEV; + + if (ps3_get_ss_laid() != PS3_SS_LAID_OTHEROS) + return -ENODEV; + return ps3_system_bus_driver_register(&ps3flash); }
The driver implements a character and misc device, meant for the axed OtherOS to exchange various settings with GameOS. Since Firmware 3.21 there is no GameOS support anymore to write these settings, so limit the driver to the OtherOS environment. Signed-off-by: Andre Heider <a.heider@gmail.com> --- arch/powerpc/platforms/ps3/Kconfig | 1 + drivers/char/ps3flash.c | 7 +++++++ 2 files changed, 8 insertions(+), 0 deletions(-)