Device models aren't supposed to go on fishing expeditions for
backends.  They should expose suitable properties for the user to set.
For onboard devices, board code sets them.

Device ssi-sd picks up its block backend in its init() method with
drive_get_next() instead.  This mistake is already marked FIXME since
commit af9e40a.

Unset user_creatable to remove the mistake from our external
interface.  Since the SSI bus doesn't support hotplug, only -device
can be affected.  Only certain ARM machines provide an SSI bus.

Signed-off-by: Markus Armbruster <armbru@redhat.com>
---
Are there valid uses of -device ssi-sd?  If no, this patch is fine.
If yes, this patch breaks them.  But fixing the FIXME will also break
them.  What should we do?

 hw/sd/ssi-sd.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
index 95a143bfba..ec7c7e6dbf 100644
--- a/hw/sd/ssi-sd.c
+++ b/hw/sd/ssi-sd.c
@@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
     k->cs_polarity = SSI_CS_LOW;
     dc->vmsd = &vmstate_ssi_sd;
     dc->reset = ssi_sd_reset;
+    /* Reason: init() method uses drive_get_next() */
+    dc->cannot_instantiate_with_device_add_yet = true;
 }
 
 static const TypeInfo ssi_sd_info = {
-- 
2.17.1

On 2018-09-26 11:00, Markus Armbruster wrote:
> Device models aren't supposed to go on fishing expeditions for
> backends.  They should expose suitable properties for the user to set.
> For onboard devices, board code sets them.
> 
> Device ssi-sd picks up its block backend in its init() method with
> drive_get_next() instead.  This mistake is already marked FIXME since
> commit af9e40a.
> 
> Unset user_creatable to remove the mistake from our external
> interface.  Since the SSI bus doesn't support hotplug, only -device
> can be affected.  Only certain ARM machines provide an SSI bus.
> 
> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
> If yes, this patch breaks them.  But fixing the FIXME will also break
> them.  What should we do?
> 
>  hw/sd/ssi-sd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 95a143bfba..ec7c7e6dbf 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
>      k->cs_polarity = SSI_CS_LOW;
>      dc->vmsd = &vmstate_ssi_sd;
>      dc->reset = ssi_sd_reset;
> +    /* Reason: init() method uses drive_get_next() */
> +    dc->cannot_instantiate_with_device_add_yet = true;

s/cannot_instantiate_with_device_add_yet = true/user_creatable = false/

Sounds like a good idea to me, if somebody then still needs this for
"-device", they should fix the FIXME first.

 Thomas

Thomas Huth <thuth@redhat.com> writes:

> On 2018-09-26 11:00, Markus Armbruster wrote:
>> Device models aren't supposed to go on fishing expeditions for
>> backends.  They should expose suitable properties for the user to set.
>> For onboard devices, board code sets them.
>> 
>> Device ssi-sd picks up its block backend in its init() method with
>> drive_get_next() instead.  This mistake is already marked FIXME since
>> commit af9e40a.
>> 
>> Unset user_creatable to remove the mistake from our external
>> interface.  Since the SSI bus doesn't support hotplug, only -device
>> can be affected.  Only certain ARM machines provide an SSI bus.
>> 
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
>> If yes, this patch breaks them.  But fixing the FIXME will also break
>> them.  What should we do?
>> 
>>  hw/sd/ssi-sd.c | 2 ++
>>  1 file changed, 2 insertions(+)
>> 
>> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
>> index 95a143bfba..ec7c7e6dbf 100644
>> --- a/hw/sd/ssi-sd.c
>> +++ b/hw/sd/ssi-sd.c
>> @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
>>      k->cs_polarity = SSI_CS_LOW;
>>      dc->vmsd = &vmstate_ssi_sd;
>>      dc->reset = ssi_sd_reset;
>> +    /* Reason: init() method uses drive_get_next() */
>> +    dc->cannot_instantiate_with_device_add_yet = true;
>
> s/cannot_instantiate_with_device_add_yet = true/user_creatable = false/

Whoops, sent the pre-rebase patch instead of the post-rebase patch.

> Sounds like a good idea to me, if somebody then still needs this for
> "-device", they should fix the FIXME first.

Peter, what do you think?

On 26 September 2018 at 12:40, Markus Armbruster <armbru@redhat.com> wrote:
> Thomas Huth <thuth@redhat.com> writes:
>
>> On 2018-09-26 11:00, Markus Armbruster wrote:
>>> Device models aren't supposed to go on fishing expeditions for
>>> backends.  They should expose suitable properties for the user to set.
>>> For onboard devices, board code sets them.
>>>
>>> Device ssi-sd picks up its block backend in its init() method with
>>> drive_get_next() instead.  This mistake is already marked FIXME since
>>> commit af9e40a.
>>>
>>> Unset user_creatable to remove the mistake from our external
>>> interface.  Since the SSI bus doesn't support hotplug, only -device
>>> can be affected.  Only certain ARM machines provide an SSI bus.
>>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
>>> If yes, this patch breaks them.  But fixing the FIXME will also break
>>> them.  What should we do?
>>>
>>>  hw/sd/ssi-sd.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
>>> index 95a143bfba..ec7c7e6dbf 100644
>>> --- a/hw/sd/ssi-sd.c
>>> +++ b/hw/sd/ssi-sd.c
>>> @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
>>>      k->cs_polarity = SSI_CS_LOW;
>>>      dc->vmsd = &vmstate_ssi_sd;
>>>      dc->reset = ssi_sd_reset;
>>> +    /* Reason: init() method uses drive_get_next() */
>>> +    dc->cannot_instantiate_with_device_add_yet = true;
>>
>> s/cannot_instantiate_with_device_add_yet = true/user_creatable = false/
>
> Whoops, sent the pre-rebase patch instead of the post-rebase patch.
>
>> Sounds like a good idea to me, if somebody then still needs this for
>> "-device", they should fix the FIXME first.
>
> Peter, what do you think?

Seems fine to me.

thanks
-- PMM

Hi Markus,

On 9/26/18 11:00 AM, Markus Armbruster wrote:
> Device models aren't supposed to go on fishing expeditions for
> backends.  They should expose suitable properties for the user to set.
> For onboard devices, board code sets them.
> 
> Device ssi-sd picks up its block backend in its init() method with
> drive_get_next() instead.  This mistake is already marked FIXME since
> commit af9e40a.
> 
> Unset user_creatable to remove the mistake from our external
> interface.  Since the SSI bus doesn't support hotplug, only -device
> can be affected.  Only certain ARM machines provide an SSI bus.

There is also a MicroBlaze machine.

Note that out of those target, we could model SSI buses on
southbridges/LPC devices but there is no interest.

I'm also spending some hobby time on a MIPS SoC which exposes a SSI bus.

> Signed-off-by: Markus Armbruster <armbru@redhat.com>
> ---
> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
> If yes, this patch breaks them.  But fixing the FIXME will also break
> them.  What should we do?

This device was probably orphan (not used) for a long time, and seems
unusable in it current state to me, see:
http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01757.html

I have to address Peter's comment to my patch noted in my TODO (low
priority), so I'm OK to get ride of the FIXME.

I suppose it is easier for you to get your patch in, then let me clean
that later.

Regards,

Phil.

> 
>  hw/sd/ssi-sd.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/hw/sd/ssi-sd.c b/hw/sd/ssi-sd.c
> index 95a143bfba..ec7c7e6dbf 100644
> --- a/hw/sd/ssi-sd.c
> +++ b/hw/sd/ssi-sd.c
> @@ -284,6 +284,8 @@ static void ssi_sd_class_init(ObjectClass *klass, void *data)
>      k->cs_polarity = SSI_CS_LOW;
>      dc->vmsd = &vmstate_ssi_sd;
>      dc->reset = ssi_sd_reset;
> +    /* Reason: init() method uses drive_get_next() */
> +    dc->cannot_instantiate_with_device_add_yet = true;
>  }
>  
>  static const TypeInfo ssi_sd_info = {
> 

Philippe Mathieu-Daudé <f4bug@amsat.org> writes:

> Hi Markus,
>
> On 9/26/18 11:00 AM, Markus Armbruster wrote:
>> Device models aren't supposed to go on fishing expeditions for
>> backends.  They should expose suitable properties for the user to set.
>> For onboard devices, board code sets them.
>> 
>> Device ssi-sd picks up its block backend in its init() method with
>> drive_get_next() instead.  This mistake is already marked FIXME since
>> commit af9e40a.
>> 
>> Unset user_creatable to remove the mistake from our external
>> interface.  Since the SSI bus doesn't support hotplug, only -device
>> can be affected.  Only certain ARM machines provide an SSI bus.
>
> There is also a MicroBlaze machine.

ssi-sd isn't linked into that one:
qemu-system-microblaze: -device ssi-sd: 'ssi-sd' is not a valid device model name

> Note that out of those target, we could model SSI buses on
> southbridges/LPC devices but there is no interest.
>
> I'm also spending some hobby time on a MIPS SoC which exposes a SSI bus.
>
>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>> ---
>> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
>> If yes, this patch breaks them.  But fixing the FIXME will also break
>> them.  What should we do?
>
> This device was probably orphan (not used) for a long time, and seems
> unusable in it current state to me, see:
> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01757.html

The more that's true, the less we have to worry about preserving
compatibility ;)

> I have to address Peter's comment to my patch noted in my TODO (low
> priority), so I'm OK to get ride of the FIXME.
>
> I suppose it is easier for you to get your patch in, then let me clean
> that later.

Works for me.

On 9/26/18 1:46 PM, Markus Armbruster wrote:
> Philippe Mathieu-Daudé <f4bug@amsat.org> writes:
> 
>> Hi Markus,
>>
>> On 9/26/18 11:00 AM, Markus Armbruster wrote:
>>> Device models aren't supposed to go on fishing expeditions for
>>> backends.  They should expose suitable properties for the user to set.
>>> For onboard devices, board code sets them.
>>>
>>> Device ssi-sd picks up its block backend in its init() method with
>>> drive_get_next() instead.  This mistake is already marked FIXME since
>>> commit af9e40a.
>>>
>>> Unset user_creatable to remove the mistake from our external
>>> interface.  Since the SSI bus doesn't support hotplug, only -device
>>> can be affected.  Only certain ARM machines provide an SSI bus.
>>
>> There is also a MicroBlaze machine.
> 
> ssi-sd isn't linked into that one:
> qemu-system-microblaze: -device ssi-sd: 'ssi-sd' is not a valid device model name
> 
>> Note that out of those target, we could model SSI buses on
>> southbridges/LPC devices but there is no interest.
>>
>> I'm also spending some hobby time on a MIPS SoC which exposes a SSI bus.
>>
>>> Signed-off-by: Markus Armbruster <armbru@redhat.com>
>>> ---
>>> Are there valid uses of -device ssi-sd?  If no, this patch is fine.
>>> If yes, this patch breaks them.  But fixing the FIXME will also break
>>> them.  What should we do?
>>
>> This device was probably orphan (not used) for a long time, and seems
>> unusable in it current state to me, see:
>> http://lists.nongnu.org/archive/html/qemu-devel/2018-06/msg01757.html
> 
> The more that's true, the less we have to worry about preserving
> compatibility ;)
> 
>> I have to address Peter's comment to my patch noted in my TODO (low
>> priority), so I'm OK to get ride of the FIXME.
>>
>> I suppose it is easier for you to get your patch in, then let me clean
>> that later.
> 
> Works for me.

So using 'user_creatable = false':
Acked-by: Philippe Mathieu-Daudé <f4bug@amsat.org>